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

chore: bump most dated deps #8850

Merged
merged 10 commits into from
Aug 20, 2019
Merged

chore: bump most dated deps #8850

merged 10 commits into from
Aug 20, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Aug 19, 2019

Summary

Now that we've dropped node 6, we can upgrade a bunch of dependencies. None of these should be breaking changes for our consumers, which is why it's a pretty big one.

Only deps held back are jsdom and micromatch, both of which I'll send separate PRs for.

I've also held source-map back as it's sync only (mozilla/source-map#331)

EDIT: Need to hold back ansi-escapes as well due to hoisting issues

Test plan

Green CI. Should probably land #8411 first though

@SimenB SimenB force-pushed the bump-deps branch 2 times, most recently from 784e2e9 to 48b020b Compare August 19, 2019 12:52
@SimenB SimenB force-pushed the bump-deps branch 5 times, most recently from d1632fa to 47d08ee Compare August 20, 2019 08:30
@SimenB
Copy link
Member Author

SimenB commented Aug 20, 2019

Getting closer. Only failure left are some weird snapshot failures.

image

They all complain about Compared values have no visual difference., not sure why.

EDIT: OH, they've trimmed a trailing newline... @pedrottimark could we improve that error? Do we run .trim() or something? See 6eeff97. Probably caused by our usage of jest-snapshot-serializer-raw

@thymikee

This comment has been minimized.

package.json Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #8850 into next will decrease coverage by 0.01%.
The diff coverage is 10%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #8850      +/-   ##
=========================================
- Coverage   63.8%   63.79%   -0.02%     
=========================================
  Files        274      274              
  Lines      11576    11589      +13     
  Branches    2841     2842       +1     
=========================================
+ Hits        7386     7393       +7     
- Misses      3560     3566       +6     
  Partials     630      630
Impacted Files Coverage Δ
packages/jest-changed-files/src/git.ts 0% <0%> (ø) ⬆️
packages/jest-changed-files/src/hg.ts 0% <0%> (ø) ⬆️
packages/jest-haste-map/src/lib/FSEventsWatcher.ts 12.5% <50%> (+1.59%) ⬆️
packages/jest-test-sequencer/src/index.ts 91.66% <0%> (+0.17%) ⬆️
packages/jest-core/src/SearchSource.ts 64.44% <0%> (+0.39%) ⬆️
packages/jest-watcher/src/lib/Prompt.ts 73.46% <0%> (+0.55%) ⬆️
...t-core/src/plugins/update_snapshots_interactive.ts 53.12% <0%> (+4.84%) ⬆️

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 9288b93...9d151f9. Read the comment docs.

@SimenB
Copy link
Member Author

SimenB commented Aug 20, 2019

Yay for mac CI, I actually broke watch mode by upgrading fsevents :P I read https://github.com/fsevents/fsevents/releases/tag/v2.0.0, without thinking about the deprecated title - the API is different in v2. I think just fsevents.watch(root, handler) is enough, but I'm not sure. I'll leave it at v1 for now, seeing as they've released a version that works on node 12

@SimenB SimenB merged commit ecac4ae into jestjs:next Aug 20, 2019
@SimenB SimenB deleted the bump-deps branch August 20, 2019 14:14
@pedrottimark
Copy link
Contributor

@SimenB Yes, your diagnosis is correct that custom serializer can contradict an original assumption that the value is enclosed in matching punctuation, like double quote marks for strings.

After const pass = expected === receivedSerialized; failed and before call to diff results:

    expected = (expected || '').trim();
    actual = (actual || '').trim();

To make sure that I understand the situation, some snapshots of CLI output had a blank line but after bump, the received values did not? After you updated them, diff told you what was the problem?

@SimenB
Copy link
Member Author

SimenB commented Aug 20, 2019

To make sure that I understand the situation, some snapshots of CLI output had a blank line but after bump, the received values did not?

Correct

After you updated them, diff told you what was the problem?

Yeah, I ran the test locally to try to figure out what was wrong, and the reporter told me to update the snapshot. So it's somewhat an issue with just the silent reporter not printing the snapshot thing (rickhanlonii/jest-silent-reporter#15), but I don't think I should be told that the values "have no visual differences" when they do. And yes, updating the snapshots and doing git diff made it obvious what the change was 🙂


I don't know what the correct solution is. Maybe look for whitespace differences if strings are being compared? Not sure

@pedrottimark
Copy link
Contributor

Oh, a candidate solution comes back to me! Instead of trim it needs to do the opposite of:

// Extra line breaks at the beginning and at the end of the snapshot are useful
// to make the content of the snapshot easier to read
const addExtraLineBreaks = (string: string): string =>
  string.includes('\n') ? `\n${string}\n` : string;

If a snapshot:

  • has at least 3 newlines
  • starts and ends with newline

then removeExtraLineBreaks slices away the first and last newline. What do you think?

@SimenB
Copy link
Member Author

SimenB commented Aug 20, 2019

Yeah, that sounds reasonable!

@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 11, 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.

5 participants