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: industrialize tests with snapshots #97

Merged
merged 6 commits into from
Apr 26, 2023

Conversation

yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented Apr 4, 2023

This PR adds the ability to test bundle and emit with snapshots. Instead of writing assertions on the output, it will be automatically compared with files already stored.

Why I did this

I would like to start fixing a few bugs (starting with #98), then contribute with my own improvements. Having snapshots makes me more confident that I will not cause a regression, and will let me see clearly the difference between the output of my revisions vs. the original output.

Why I did not use assertSnapshot from the standard library

There are already ways of doing snapshot testing from what std provides. However, with the existing assertSnapshot, the snapshot files are big and hard to read. The functions of deno_emit produce JavaScript code, so I preferred to store the snapshots as JavaScript files. With the absence of escape characters and the help of syntax highlighting, we can better appreciate the changes.

Other changes

The existing tests have been converted to using snapshots. Tests of bundle and tests of emit have been separated in two different files.

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

@dsherret dsherret changed the title Industrialize tests with snapshots test: industrialize tests with snapshots Apr 4, 2023
@dsherret
Copy link
Member

dsherret commented Apr 4, 2023

@yacinehmito seems some tests are failing.

@yacinehmito
Copy link
Contributor Author

yacinehmito commented Apr 4, 2023

I think it's because the source maps depend on something device-specific (probably the full path). I'll dig further.

@yacinehmito yacinehmito force-pushed the snapshots branch 2 times, most recently from 53fde10 to baeca02 Compare April 9, 2023 06:17
@yacinehmito
Copy link
Contributor Author

@dsherret Tests are fixed.

@yacinehmito
Copy link
Contributor Author

yacinehmito commented Apr 9, 2023

I added functions to "fix" the source maps, so that they use relative paths instead of absolute paths.
This is a bit hacky; I would have preferred to do this Rust-side, but the logic is deep in deno_ast so that wasn't a practical option.

@yacinehmito
Copy link
Contributor Author

@dsherret Sorry for pinging you again.

@dsherret
Copy link
Member

@yacinehmito the CI is still failing. I fixed the one newline issue.

@yacinehmito
Copy link
Contributor Author

I didn't realize that there was a windows run. I am not surprised if it fails because of line endings given how I split the lines in the testing code. It's a bit hard to test on my end as I cannot trigger the CI.

I'll push a fix early next week (I am off until then).

@dsherret
Copy link
Member

I can try to fix it in a bit (maybe later tonight or tomorrow). Yeah, the CI doesn't run automatically because this is your first PR to this repo. I wish it would after the first time I approve it.

tests/utils.ts Outdated

const osType = typeof Deno?.build?.os === "string" ? Deno.build.os : "linux";

const EOL = osType === "windows" ? "\r\n" : "\n";
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 have added this bit and used it everywhere instead of \n. @dsherret Does that make the tests pass now?

Copy link
Member

@dsherret dsherret Apr 21, 2023

Choose a reason for hiding this comment

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

I had fixed that issue by adding a .gitattributes to this PR, which is preferable because then we don't need these kind of conditions. The tests were complaining about this:

,-     "file:///D:/a/deno_emit/deno_emit/testdata/mod.ts": "cYfK3r_ND0Q_nXVQYOxJjg8VYMM.js"\n
,+     "file:///testdata/mod.ts": "0BJ1Cp_wwHSv6YnJzSlY2UlxpSc.js"\n

Previous run: https://github.com/denoland/deno_emit/actions/runs/4726314879/jobs/8385766439

Now the tests are failing with more newline stuff. Can you revert your last change (I have the old code locally in case you've lost it so just let me know if you want me to push it to this PR)? Also, please land commits on top of each other instead of overwritting commits because I don't know what has changed since the last time I reviewed. We'll squash all the commits when merging.

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 reverted the changes. I also found how to run the Actions on my own fork, so I can iterate on the Windows issue.

(FYI, the Windows build takes a lot of time. Compiling Rust dependencies takes more than 12 minutes, whereas it takes less than 5 minutes for Linux and less than 2 minutes for macOS.)

@yacinehmito yacinehmito force-pushed the snapshots branch 2 times, most recently from c150000 to 001113b Compare April 24, 2023 18:16
@yacinehmito
Copy link
Contributor Author

We're almost there. There's only one test failing, because of line endings.

@yacinehmito
Copy link
Contributor Author

@dsherret It's finally green on my fork! 😄

There's no need to wait until the end of the process.
@yacinehmito
Copy link
Contributor Author

I removed the queue to update the snapshots so that I could forward the paths to the module outputs to the more function, which can be used to perform more assertions.

I did that because I am porting the tests from deno bundle in here, and some of them need to execute the bundled code. I figured that I could directly get it from the snapshot instead of bundling another time in a temporary directory.

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