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] Treat snapshots as related to test files #3025

Merged
merged 1 commit into from
Mar 1, 2017

Conversation

thymikee
Copy link
Collaborator

Summary

Fixes #2843.
Feel free to close it, if we don't want such behaviour, but I think it's nice.

Test plan

jest-resolve-dependencies has no tests :(. If we eventually want to merge it, I'll add some.

@codecov-io
Copy link

Codecov Report

Merging #3025 into master will decrease coverage by -0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3025      +/-   ##
==========================================
- Coverage   68.49%   68.45%   -0.04%     
==========================================
  Files         146      146              
  Lines        5335     5338       +3     
==========================================
  Hits         3654     3654              
- Misses       1681     1684       +3
Impacted Files Coverage Δ
packages/jest-resolve-dependencies/src/index.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9874b81...2c9f715. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Mar 1, 2017

Yap, this is definitely a step in the right direction to make -o more useful!

@cpojer cpojer merged commit b9feb4a into jestjs:master Mar 1, 2017
@cpojer
Copy link
Member

cpojer commented Mar 1, 2017

Nice work as always @thymikee. I think we should consider doing something unpopular: create a jest-snapshot-support package that has all the constants and utility functions. I don't want all of the snapshot package to end up all over Jest but it seems like we copied constants around and sharing those would be useful. What do you think?

@thymikee thymikee deleted the snapshot-related-tests branch March 1, 2017 06:54
@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

Agreed, jest-snapshot-support may come in handy and it wouldn't require to load whole jest-snapshot

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

Master still fails for me using my repro...

@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

I've tested this on your repo. Are you sure you're using latest master build?

snap

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

Yeah, I think so. How do you create the gif? :D

@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

I use LICEcap http://www.cockos.com/licecap/

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

Awesome!

jest

When the test fails at the end, I've pressed a instead of enter in the window

@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

This is odd, same is working for me. I tend to build using yarn or yarn install on Jest repo – maybe that helps?

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

Seeing as build is just node ./scripts/build.js I would hope not... I did the install using yarn. I can try cloning a fresh Jest install

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

Nope, still fails. Is it watchman vs not watchman?
After testing, same thing happens.

Being quick with a ^C allows me to do --debug --watch

jest version = 19.0.2
test framework = jasmine2
config = {
  "automock": false,
  "bail": false,
  "browser": false,
  "cacheDirectory": "/var/folders/89/vbf7hhns3m9g138cgy1hzlrx17ddry/T/jest",
  "clearMocks": false,
  "coveragePathIgnorePatterns": [
    "/node_modules/"
  ],
  "coverageReporters": [
    "json",
    "text",
    "lcov",
    "clover"
  ],
  "expand": false,
  "globals": {},
  "haste": {
    "providesModuleNodeModules": []
  },
  "moduleDirectories": [
    "node_modules"
  ],
  "moduleFileExtensions": [
    "js",
    "json",
    "jsx",
    "node"
  ],
  "moduleNameMapper": {},
  "modulePathIgnorePatterns": [],
  "noStackTrace": false,
  "notify": false,
  "preset": null,
  "resetMocks": false,
  "resetModules": false,
  "roots": [
    "/Users/simbekkh/repos/jest-snapshot-cache-bug"
  ],
  "snapshotSerializers": [],
  "testEnvironment": "jest-environment-jsdom",
  "testMatch": [
    "**/__tests__/**/*.js?(x)",
    "**/?(*.)(spec|test).js?(x)"
  ],
  "testPathIgnorePatterns": [
    "/node_modules/"
  ],
  "testRegex": "",
  "testResultsProcessor": null,
  "testURL": "about:blank",
  "timers": "real",
  "transformIgnorePatterns": [
    "/node_modules/"
  ],
  "useStderr": false,
  "verbose": null,
  "watch": true,
  "rootDir": "/Users/simbekkh/repos/jest-snapshot-cache-bug",
  "name": "d45f3bd6d680be0d353c9904d641fd54",
  "setupFiles": [],
  "testRunner": "/Users/simbekkh/repos/jest-snapshot-cache-bug/node_modules/jest-jasmine2/build/index.js",
  "transform": [
    [
      "^.+\\.jsx?$",
      "/Users/simbekkh/repos/jest-snapshot-cache-bug/node_modules/babel-jest/build/index.js"
    ]
  ],
  "cache": true,
  "watchman": true
}

@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

And what happens when you run jest -o?

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

Is the transform part bad? It points to my locally installed, not the one I run.

$ ../jest/packages/jest/bin/jest.js -o
No tests found related to files changed since last commit.
Run Jest without `-o` to run all tests.

With this diff

diff --git i/__snapshots__/test.js.snap w/__snapshots__/test.js.snap
index f81294f..2fffbbe 100644
--- i/__snapshots__/test.js.snap
+++ w/__snapshots__/test.js.snap
@@ -2,6 +2,6 @@
 
 exports[`something 1`] = `
 Object {
-  "input": 42,
+  "output": 42,
 }
 `;

@SimenB
Copy link
Member

SimenB commented Mar 1, 2017

That was it! I had to remove my node_modules, as jest resolved deps from there instead of from itself. False alarm then, the bug appears fixed. Sorry about the noise 😄

@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

Yeah! Awesome to hear that. btw, there's npm run clean-all to purge all node_modules, just in case 🙂

@thymikee
Copy link
Collaborator Author

thymikee commented Mar 1, 2017

And once you npm link a package you need to be extra careful with unlinking it from local and global deps. It's a pain. Hopefully the whole process will be easier with yarn knit https://twitter.com/cpojer/status/836730384620617729

skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watch mode doesn't consider changes to snapshot files changes
5 participants