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: use snapshot #151

Merged
merged 3 commits into from
Aug 21, 2024
Merged

Conversation

marcalexiei
Copy link
Contributor

@marcalexiei marcalexiei commented Jul 25, 2024

Part of #141


Unit tests updates

Snapshots

Snapshots are used whenever is possible.

Warning

It was not possible to use them in all tests.

Detailed explanation

watchlist

Can't use snapshot since is comparing rollup properties with file system files.
However I was able to simplify the logic (I created a separate commit).

output

Using snapshot here is not feasible due to rollup:
It looks like that, randomly, plugin transform is called first on actual_b and then on actual_a instead of calling transform on actual_a and then on actual_b.

So instead of having:

body{color:red}body{color:green}

you end up having

body{color:green}body{color:red}

But only some times.

This looks like a rollup bug:

Right now on main branch should support output as function sometimes fails for the exact same reason.
Also on other tests involving output, assertions are written using indexOf !== -1 logic.
I assume it's because of this bug.

E.g.:

.then((rslt) => {
t.not(squash(rslt.toString()).indexOf(expectA), -1);
t.not(squash(rslt.toString()).indexOf(expectB), -1);
});

Right now there isn't much we can do.
I'm going to add a code comment on options.output.test.ts referencing this PR

insertStyle.test.ts – replace jsdom with happy-dom

Since the test are quite simple I opted out to switch jsdom to happy-dom (less dependencies and faster)

Additional changes

  • All unit test dependencies have been updated:
    • ava
    • @ava/typescript
    • nyc
    • sinon
  • Added missing @types for
    • sinon
    • icss-utils

@marcalexiei
Copy link
Contributor Author

All review comments of #143 have been applied in this PR

@marcalexiei marcalexiei force-pushed the feature/test-snapshot-updates branch from 5196988 to 3cd38e9 Compare August 3, 2024 21:11
@elycruz elycruz changed the base branch from main to dev August 7, 2024 03:22
Copy link
Owner

@elycruz elycruz left a comment

Choose a reason for hiding this comment

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

Left a couple of comments - Review of test/index.test.ts still pending.

test/insertStyle.test.ts Show resolved Hide resolved

test("insertStyle shouldn't choke when window is undefined", (t) => {
delete global["window"];
t.throws(() => !window);
Copy link
Owner

Choose a reason for hiding this comment

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

throw test seems un-required here - reasoning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only guess the reason... this assertion is already present on the main branch

t.throws(() => !window);

the tests are probably executed in strict mode so an error occurs if you access a variable that doesn't exists

Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to track these *.snap files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/avajs/ava/blob/main/docs/04-snapshot-testing.md

Say you have ~/project/test/main.js which contains snapshot assertions. AVA will create two files:

  • ~/project/test/snapshots/main.js.snap
  • ~/project/test/snapshots/main.js.md

The first file contains the actual snapshot and is required for future comparisons.
The second file contains your snapshot report. It's regenerated when you update your snapshots.
If you commit it to source control you can diff it to see the changes to your snapshot.

@marcalexiei
Copy link
Contributor Author

@elycruz yesterday you made changes to test on main via #153 leading to conflicts with the work I done here.
Snapshot test update PRs has been opened for nearly 2 months (#143) so could you please complete the review?
If you changed your mind feel free to close this.

@marcalexiei marcalexiei changed the base branch from dev to main August 20, 2024 12:17
@elycruz elycruz changed the base branch from main to dev August 21, 2024 13:28
@elycruz elycruz self-assigned this Aug 21, 2024
@elycruz elycruz merged commit 0ec8148 into elycruz:dev Aug 21, 2024
3 checks passed
@elycruz
Copy link
Owner

elycruz commented Aug 21, 2024

@marcalexiei Hey, right - apologies about that - have been uber swamped lately -
Not a problem - I'm merging it into the 'dev' branch (will make some annotations) and will add you to the subsequent PR -
I'll correct/make-small-updates for annotations I'm making and then after you review those changes we can merge it.

@marcalexiei marcalexiei deleted the feature/test-snapshot-updates branch August 21, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants