Skip to content

Add tests#17

Open
BigBlueHat wants to merge 13 commits intomainfrom
add-tests
Open

Add tests#17
BigBlueHat wants to merge 13 commits intomainfrom
add-tests

Conversation

@BigBlueHat
Copy link
Copy Markdown
Contributor

@BigBlueHat BigBlueHat commented Mar 24, 2026

  • Add mocha/chai tests for stdin, files, & HTTP.
  • Use deep.equal instead of item checks.
  • Remove timeout.
  • Remove use of old school aliases.
  • Rename folder from fixtures to data.
  • Make test input data more real.

Copy link
Copy Markdown
Member

@davidlehn davidlehn left a comment

Choose a reason for hiding this comment

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

  • Should add a github action to run these.
  • Is "fixture" term use ok? I thought that was more the broader testing setup vs only the test data here. Can be changed later if needed.
  • Should perhaps target a different branch. It's overlapping with other PRs.

@BigBlueHat
Copy link
Copy Markdown
Contributor Author

  • Should perhaps target a different branch. It's overlapping with other PRs.

Ah. I see. I had the branches stacked locally, but GitHub ignores that when you make a PR... It's possible to later edit that (or maybe set it via the CLI), but I forgot to do that. Sorry for the confusion.

@BigBlueHat BigBlueHat requested a review from davidlehn March 26, 2026 18:09
@BigBlueHat BigBlueHat changed the base branch from main to fix-vulns March 26, 2026 18:11
@BigBlueHat
Copy link
Copy Markdown
Contributor Author

@davidlehn GitHub Action added. Obligatory CHANGELOG.md conflict added also...

@BigBlueHat BigBlueHat changed the base branch from fix-vulns to main March 31, 2026 20:29
@BigBlueHat
Copy link
Copy Markdown
Contributor Author

Should perhaps target a different branch. It's overlapping with other PRs.

I've changed the base to main which gets around the CHANGELOG.md conflict...until #16 is merged.

@BigBlueHat BigBlueHat changed the title add tests Add tests Mar 31, 2026
@davidlehn
Copy link
Copy Markdown
Member

  • Should perhaps target a different branch. It's overlapping with other PRs.

Ah. I see. I had the branches stacked locally, but GitHub ignores that when you make a PR... It's possible to later edit that (or maybe set it via the CLI), but I forgot to do that. Sorry for the confusion.

Hit the title edit button and you can change the target branch.

Copy link
Copy Markdown
Member

@davidlehn davidlehn left a comment

Choose a reason for hiding this comment

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

Getting a bit confused with the multiple prs and what was changed where. Like the node_modules ignore is here too.

run: npm run lint
test:
runs-on: ubuntu-latest
timeout-minutes: 360
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

360 is the default. I'd hope tests don't take that long! Maybe this should be lower or removed if default is fine.

@@ -10,12 +12,34 @@ jobs:
matrix:
node-version: [20.x]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
node-version: [20.x]
node-version: [24.x]

timeout-minutes: 360
strategy:
matrix:
node-version: [20.x]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
node-version: [20.x]
node-version: [20.x, 22.x, 24.x]

Comment on lines +40 to +41
data.should.be.an('object');
data.should.have.property('@context');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be a full check too right?

Suggested change
data.should.be.an('object');
data.should.have.property('@context');
data.should.deep.equal(JSON.parse(fixtureData));

Comment on lines +73 to +74
parsed.should.be.an('object');
parsed.should.have.property('@context');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be a full check too, right?

Suggested change
parsed.should.be.an('object');
parsed.should.have.property('@context');
parsed.should.deep.equal(JSON.parse(fixtureData));

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.

2 participants