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

PR for #29 #33

Closed
wants to merge 13 commits into from
Closed

PR for #29 #33

wants to merge 13 commits into from

Conversation

andrestaht
Copy link
Collaborator

Didn't bother rewriting the tests yet because this idea might get scrapped.
Probably should make createState and removeUncheckedKeys accessible like other functions are defined in index.js.
With current state creating ci and update could be removed form determineConfig.js.

My mochaSetup.js:

chai.use(chaiJestSnapshot);

before(function() { // eslint-disable-line no-undef
    chaiJestSnapshot.resetSnapshotRegistry();
});

beforeEach(function() {
    // Save snapshots to __snapshots__ dir
    const filename = path.resolve(path.dirname(this.currentTest.file), "__snapshots__", path.basename(this.currentTest.file) + ".snap");

    chaiJestSnapshot.setFilename(filename);
    chaiJestSnapshot.setTestName(this.currentTest.fullTitle());
    chaiJestSnapshot.createState({
        snapshotPath: filename,
        ci: process.env.CI,
        update: process.env.CHAI_JEST_SNAPSHOT_UPDATE_ALL,
    });
});
after(function() { // eslint-disable-line no-undef
    chaiJestSnapshot.removeUncheckedKeys();
});

Didn't bother rewriting the tests yet because this idea might get scrapped.
Probably should make createState and removeUncheckedKeys accessible like other functions are defined in index.
With current state creating ci and update could be removed form determineConfig.js.
@suchipi
Copy link
Owner

suchipi commented Mar 22, 2018

Thanks, I'll take a look soon

@andrestaht
Copy link
Collaborator Author

@suchipi did you get a chance to look into this? :)

@suchipi
Copy link
Owner

suchipi commented Apr 16, 2018

Sorry, I looked at it briefly but have not had a chance to follow back up, I apologize for the long delay.

I don't think there's a straightforward way to support both matchSnapshot(true) and the snapshot state from jest, so I think we should move to the latter and make this a breaking change.

@@ -65,6 +66,9 @@ chaiJestSnapshot.resetSnapshotRegistry = function resetSnapshotRegistry() {
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add the stuff you had in your beforeEach to configureUsingMochaContext?

@@ -65,6 +66,9 @@ chaiJestSnapshot.resetSnapshotRegistry = function resetSnapshotRegistry() {
}
}

chaiJestSnapshot.createState = snapshotStateHandler.create;
chaiJestSnapshot.removeUncheckedKeys = snapshotStateHandler.removeUncheckedKeys;
Copy link
Owner

Choose a reason for hiding this comment

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

We will have to refactor the documentation to note that you need to call these in your beforeAll/afterAll.

let states = {};

module.exports = {
create: ({snapshotPath, ci, update}) => {
Copy link
Owner

Choose a reason for hiding this comment

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

The ci and update arguments can stay internal here (hardcoded to the process environment variables); the public interface should only take the path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! :)

@andrestaht
Copy link
Collaborator Author

Thanks for the review!
Pushed some changes.

There are still couple of things missing:

  1. snapshotStateHandler isn't covered with tests
  2. ci and update aren't tested in matchSnapshot

Andres Täht added 2 commits April 18, 2018 14:24
Not the best test coverage but should give an overview what is going on.
@andrestaht
Copy link
Collaborator Author

@suchipi could you take another look at this?
Didn't find a good way to test ci and update flags within matchSnapshot tests but added some unit tests for snapshotStateHandler.

@suchipi
Copy link
Owner

suchipi commented May 2, 2018

Hey, sorry for the radio silence on this. I will try to take a look soon.

@suchipi suchipi self-assigned this May 2, 2018
@andrestaht
Copy link
Collaborator Author

Hey, @suchipi! Could you have a look at this? :)

@suchipi
Copy link
Owner

suchipi commented Jun 19, 2018

Hey, I'm sorry this is still open. I took a look 22 days ago when you commented and started working through behavior and thinking about how it should work, but I didn't have time to finish. I'll let you know when I look at it again- sorry again and thank you for being so patient 🙏

@suchipi
Copy link
Owner

suchipi commented Jun 28, 2018

@andrestaht are you interested in me giving you write access to this repo and you being a co-maintainer of the package?

@andrestaht
Copy link
Collaborator Author

@suchipi why not :)

@mAAdhaTTah
Copy link

I'm interested in this feature as well. Anything I can do to move this along?

@fa93hws
Copy link

fa93hws commented Jul 14, 2019

Any update?
Willing to give a hand as well.

@andrestaht
Copy link
Collaborator Author

Sorry for the silence. I switched companies and this dependency is no longer part of the stack.

If someone still sees value in this then feel free to take it over.

I'll help with the review and getting the PR released.

Sorry again for this long unactivity.

@andrestaht andrestaht closed this Oct 6, 2020
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.

None yet

4 participants