Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(pageservice): add tests for page service #588

Merged
merged 16 commits into from
Apr 6, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Dec 12, 2022

Problem

previously, PageService had no associated tests; this PR add the tests in.

Solution

  1. write unit tests for pageService
    • the unit tests are centered around parsePageName together with retrieveStagingPermalink; the invariants that we want to guarantee will be briefly outlined below
    • for static pages (pages whose name are always fixed), maintain the invariant that only that specified filepath can be parsed into that name
    • guarantee that for resource rooms, we will only get permalink for posts
    • recursive descent (ie, the order in which we attempt to parse is fixed)
    • note that filepaths terminating in / will not have a name property - consistent w/ how directories behave; we could extend our model to also parse directories but this is not done at present. this should also not be possible if we are receiving from the fe

Copy link
Contributor

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Briefly looked through the test cases, seems ok to me.

src/constants/pages.ts Outdated Show resolved Hide resolved
src/services/fileServices/MdPageServices/PageService.ts Outdated Show resolved Hide resolved
// Arrange
const expected = ok({
name: HOMEPAGE_NAME,
kind: "Homepage",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this concerns mainly the original PR, but should we change this to use an enum instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually i ran into troubles with using enums as types in the past and hence, i didn't use it here. i think the original PR has a comment about this raised by kishore also

@seaerchin seaerchin requested a review from dcshzj December 15, 2022 08:03
@seaerchin seaerchin force-pushed the test/page-service branch from 42be4e6 to 07701ac Compare April 6, 2023 08:38
@seaerchin seaerchin merged commit 71f0cac into test/buttons Apr 6, 2023
@seaerchin seaerchin deleted the test/page-service branch April 6, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants