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

Add tests for internal linking #808

Merged
merged 3 commits into from
Feb 17, 2024
Merged

Add tests for internal linking #808

merged 3 commits into from
Feb 17, 2024

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Feb 11, 2024

Since we mentioned tests in #804 and I noticed internal linking is a good candidate to start adopting tests, I added the simplest way to include tests for it that I could imagine.

This only uses the node internal assert package. No test framework required.

I especially like how the tests document what behavior we want to see and prevent regressions.

Some drawbacks:

  • we can't test code that includes JSX
  • Using node **/*.spec.js works with multiple test files but on first error, the whole test suite is aborted.

Errors would look like this:

$ npm test

> [email protected] test
> node **/*.spec.js

node:internal/process/esm_loader:34
      internalBinding('errors').triggerUncaughtException(
                                ^

AssertionError [ERR_ASSERTION]: https://stacker.news/items/123foobar - expected: undefined got: #123foobar
    at file:///home/ekzyis/programming/stacker.news/lib/url.spec.js:22:5
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Node.js v21.4.0

This is based on #807 and should thus be merged after.

@ekzyis ekzyis added documentation improvements or additions to documentation feature new product features that weren't there before simple enhancement improvements to existing features local dev labels Feb 11, 2024
@alexlwn123
Copy link
Contributor

Awesome! Happy to see tests being worked on.

Is there any downside to using a testing framework like jest?

For testing jsx, we can use something like the React Testing Library. I'm currently working on a PR that sets this up.

Linking test cases look great.

@huumn huumn removed the simple label Feb 14, 2024
@ekzyis
Copy link
Member Author

ekzyis commented Feb 15, 2024

Is there any downside to using a testing framework like jest?

No downside, just wanted to keep the first draft simple and didn't want to decide on a testing framework without discussion. But ime, jest is the standard for React apps so I agree with using jest.

Will convert the code to use jest soon.

Copy link
Contributor

@alexlwn123 alexlwn123 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] Transitive: environment, eval, filesystem, network, shell, unsafe +277 21.9 MB simenb

View full report↗︎

@ekzyis
Copy link
Member Author

ekzyis commented Feb 17, 2024

Test output:

$ npm run test

> [email protected] test
> NODE_OPTIONS='--experimental-vm-modules' jest

(node:2758185) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
 PASS  lib/url.spec.js
  internal links
    ✓ parses "https://stacker.news/items/123" as "#123" (2 ms)
    ✓ parses "https://stacker.news/items/123/related" as "#123/related"
    ✓ parses "https://stacker.news/items/123foobar" as undefined
    ✓ parses "https://stacker.news/items/123/r/ekzyis" as "#123"
    ✓ parses "https://stacker.news/items/123?commentId=456" as "#456"
    ✓ parses "https://stacker.news/items/123/r/ekzyis?commentId=456" as "#456"
    ✓ parses "https://stacker.news/items/123?commentId=456&parentId=789" as "#456"

Test Suites: 1 passed, 1 total
Tests:       7 passed, 7 total
Snapshots:   0 total
Time:        0.194 s, estimated 1 s
Ran all test suites.

LGTM!

You are fast haha

@huumn huumn merged commit 6e6c355 into master Feb 17, 2024
4 checks passed
@ekzyis ekzyis deleted the fix-internal-linking-tests branch March 27, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation improvements or additions to documentation enhancement improvements to existing features feature new product features that weren't there before local dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants