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 and split into multiple files #143

Closed
wants to merge 5 commits into from

Conversation

marcalexiei
Copy link
Collaborator

@marcalexiei marcalexiei commented Jul 3, 2024

Part of #141


Unit tests updates

Split tests between files

Unit tests have been divided into multiple files and now use snapshots.
This should improve readability and, hopefully, maintenance efforts.
All common utilities and variables are now exported by test/utils/index file

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

@elycruz
Copy link
Owner

elycruz commented Jul 6, 2024

Started looking over the changes - Will review them fully over the weekend.

elycruz added a commit that referenced this pull request Jul 6, 2024
- Added 'husky', and 'commitlint'
  dev-deps.
- Ran 'npx husky init' (which
  in-turn, creates initial
  'pre-commit' hook).
elycruz added a commit that referenced this pull request Jul 6, 2024
- Added 'commit-msg' hook,
  and (package) script.
elycruz added a commit that referenced this pull request Jul 6, 2024
- Removed un-required
  changes.
@elycruz elycruz added enhancement CI/CD Issues related to running, and/or, updating ci/cd processes. and removed CI/CD Issues related to running, and/or, updating ci/cd processes. labels Jul 6, 2024
@elycruz
Copy link
Owner

elycruz commented Jul 13, 2024

@marcalexiei Hey, the changes are pretty lenghty to go through - Could you summarize which tests have been updated to use snapshots, and which tests actually changed (I see a couple are using snapshots, but not all of them)?

Copy link
Collaborator Author

@marcalexiei marcalexiei left a comment

Choose a reason for hiding this comment

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

The majority of test has been converted to snapshot, the change is basically replace one or more assertions with t.snapshot


Here is the list of list which are not using snapshot and why:

  • test/insertStyle.test.ts do not use snapshot because is testing just the function itself
  • test/options.insert.test.ts
  • test/sourcemap.test.ts doesn't use snapshots because are testing sourcemaps

After reviewing the changes I noticed tests in these files probably can use snapshots

  • test/options.output.test.ts
  • test/watchList.test.ts

I'll update them now.

@marcalexiei
Copy link
Collaborator Author

test/watchList.test.ts

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).

test/options.output.test.ts

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

@elycruz
Copy link
Owner

elycruz commented Jul 18, 2024

I'm glad you noticed that bug (I had noticed it too) - Lets connect on the interwebs to discuss this PR - I'm on discord (as edlcmoose) - Note: I'm only asking because this is a large change that we should probably do in smaller pull requests also the value of changing all tests to use snapshots isn't very clear to me so was hoping you could shed light on what the pros/cons are about adapting this approach, and/or if there are any analysis that you have performed on the approach.

@marcalexiei
Copy link
Collaborator Author

For transparency and because I do not have a Discord account 😅 I will reply here:

I'm only asking because this is a large change that we should probably do in smaller pull requests

  • I divided the monolithic file into different files but the tests are the same.
  • I haven't changed test description, so it can be compared with the previous implementation.
  • I had to move the variables that were on the top of index.test.ts inside a separate file (test/utils/index.ts) to avoid duplication between the new test files.
  • The variables have been renamed with a TEST_ prefix to avoid clash with src folder export.

I do not have any rush to get this merged so feel free to review when you have time 😀.

Note

If you stil find hard to review the changes I can close this PR and re-create smaller ones.


the value of changing all tests to use snapshots isn't very clear

IMHO The main benefit is that though the snapshots you have a quick and fast way to compare the string output instead of having to search around for "expect" files that are read through node:fs and compared to plugin output after various trim / clean operations.


if there are any analysis that you have performed on the approach

I used snapshot approach for the first time when implementing a custom webpack loader tests.
I find it more quick and easy than manipulating the strings directly via JS (which I was doing previously).

An additional benefit is that they can easily updated via cli command instead of performing the operation manually.

You can find the code here: https://github.com/marcalexiei/ractive-html-loader
The snapshot are used in this file: test/loader.test.js


@elycruz
Copy link
Owner

elycruz commented Jul 19, 2024

Ok, thank you for indulging me - I understand the changes better now -
will set time to review and validate them over the next couple of days.

@marcalexiei
Copy link
Collaborator Author

thank you for indulging me

No problem 😉

will set time to review and validate them over the next couple of days.

Thank you! As I said there is no rush 😃.


Have a good weekend!

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.

Ok, so generally your changes look good, however they are to many to consume in one PR - You will need to separate it as follow, as well as make the review updates mentioned in the other review comments:

  1. One PR for dependency updates.
  2. One for changing all tests to use snapshots, and/or just contain updates, and,
  3. One for breaking the tests out into their own files.

Like this we would be able to consume the changes without requiring high cognitive load (too many things changing requires more things to keep in mind).


import sass from "../src/index";
import {
TEST_UTILS,
Copy link
Owner

Choose a reason for hiding this comment

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

So for TEST_UTILS, could you change the exports to just the methods themselves - Since they already live at 'test/utils' it becomes redundant to export the object TEST_UTILS - What would be good here would be to rename squash to stripNewLines, or something similar!

Copy link
Owner

Choose a reason for hiding this comment

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

Also, getOutputFirstChunkCode could just be getFirstChunkCode - since the declaration would read getFirstChunkCode(output: ...) ... - Would also change output to chunks, and/or, outputChunks for more correlation between what output constants actually represent.

@elycruz
Copy link
Owner

elycruz commented Jul 24, 2024

@marcalexiei Also, the dependency updates can go together with the changing the tests PR.

@marcalexiei
Copy link
Collaborator Author

Ok...

@marcalexiei marcalexiei deleted the feature/test-snapshot branch July 25, 2024 05:01
@marcalexiei marcalexiei mentioned this pull request Jul 25, 2024
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