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

[WIP] bun test to support snapshots #1692

Closed
wants to merge 15 commits into from

Conversation

ethanburrell
Copy link
Contributor

@ethanburrell ethanburrell commented Dec 31, 2022

Adds support for snapshots by bun-test as proposed in #1642 and #1825.

This PR implements the following function:

expect(...).toMatchSnapshot(hint?: string): void

This is very similar to Jest's toMatchSnapshot, a tool to test if a visual component or object has changed. If a file needs to have a new snapshot generated then pass the --updateSnapshot argument to bun wiptest and a new snapshot file will be generated.

There are two key differences between bun's implementation of toMatchSnapshot and Jest's implementation:

  1. The formatting of snapshot files is provided by bun's formatter and not the one that Jest uses which leads to objects to be stringified in a different format than in Jest. If migrating from a Jest codebase you will need to --updateSnapshot for your tests to pass.

  2. This does not currently support the Property Matchers argument. This will be updated after some features are added to wiptest.

Open questions for reviewers:

  1. First time writing a major feature in Zig! Any style or conventions tips would be appreciated.
  2. Would like some guidance on how to handle mis-formatted snapshot files. See comment below

@ethanburrell
Copy link
Contributor Author

I need to unblock #1533 and #1591 to move forward with the next parts of this ticket.

if (_arguments.len == 1) {
hint = arguments[0];
} else if (_arguments.len > 1) {
globalObject.throw("toMatchSnapshot does not support `propertyMatchers` yet.", .{});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

propertyMatchers need something like the Expect Any Constructor to work correctly.

const arguments = [1]js.JSValueRef{expect_arg};

// Call the function
_ = JSC.C.JSObjectCallAsFunction(globalObject, function, null, 1, &arguments, exception_ptr) orelse unreachable;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function parseSnapshot uses the same logic as jest to parse the snapshot file.

Currently if the snapshot file is mis-formatted this will cause a segfault in bun. How can I add error handling so the test can fail normally if the snapshot is mis-formatted?

Copy link
Contributor Author

@ethanburrell ethanburrell Feb 1, 2023

Choose a reason for hiding this comment

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

Overview of how this function works:

If a snapshot file is formatted like:

exports[`test that the string is five`] = `5`;

The jest library / this snapshot logic will run the equivalent javascript code

      snapshotContents = fs.readFileSync(snapshotPath, 'utf8');
      // eslint-disable-next-line no-new-func
      const populate = new Function('exports', snapshotContents);
      populate(data);

Which will transform the data into an object that looks like the following:

exports = { `test that the string is five`: `5` }

However when the input snapshot file is incorrectly formatted it can cause the bun to segfault. For example if the snapshot file saved looks like:

`test that the string is five` = `5`;

Then the program will segfault at snapshot.zig:149. How can catch an exception from the zig call to JSObjectCallAsFunction? This is equivalent to the const populate = new Function('exports', snapshotContents); line from the Jest program.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this needs to be JavaScript source code

We can print it to look like JavaScript code to match Jest behavior, but assuming snapshots are always plain text - it could be a file parsed with Bun's JS parser and then printed with Bun's JS printer afterwards.

There are several garbage collector things in here that will cause segfaults in the current implementation

@@ -0,0 +1,15 @@
# `bun wiptest`
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 don't know where to add docs on how to use this.

}

pub fn exists(this: *SnapshotFile) bool {
return (fs.FileSystem.FilenameStore.instance.exists(this.path.text) or
Copy link
Collaborator

Choose a reason for hiding this comment

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

We unfortunately can't use exists from FilenameStore or DirnameStore here because that function checks if the path string was allocated into it, not anything to do with the filesystem. Those stores are big chunks of preallocated memory for storing file path strings

pub const CountersMap = std.HashMap(u64, u32, IdentityContext(u64), 80);

pub fn readAndParseSnapshot(this: *SnapshotFile, globalObject: *JSC.JSGlobalObject) void {
var snapshot_contents = this.readSnapshot() catch return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This string is potentially a UTF-8 string that needs to be re-encoded (call .withEncoding())

if (not) pass = !pass;
if (pass) return true;

var fmt = JSC.ZigConsoleClient.Formatter{ .globalThis = globalObject };
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner Feb 1, 2023

Choose a reason for hiding this comment

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

I'm not sure that we can use the console.log formatter in here. Is that what Jest does? Bun's console.log formatter doesn't deeply visit every nested object and it doesn't visit every JSX child element

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

Thank you very much for this

There are two main issues with this approach:

  1. Using JavaScript wrapper functions for exports means that everything in/out of the snapshots need to be reachable by the garbage collector - which means its very easy to cause segfaults. Instead, the keys and values should be UTF-8 string. If we need to make the snapshot format match Jest (which means parsing JS), since the values are exports[identifier] = 'string literal' this can be done by bun's JS parser without invoking JS at all

  2. To capture the snapshot, we need to do something that's not quite using the rest of the console.log code. console.log doesn't deeply snapshot objects. We probably should use something like JSON.stringify except with a way to handle Symbol, JSX, ignore functions and other things which are not clonable. For this, it would be good to look at Jest's implementation and then possibly implement it in C++ (potentially similar to Bun__deepEquals except instead of comparing equality, it would write the snapshot string?)

@Electroid
Copy link
Contributor

Thanks for working on this. We recently added support for snapshots.

@Electroid Electroid closed this Mar 29, 2023
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