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

upgrade JSDOM to 15.2.1 #12704

Merged
merged 1 commit into from
May 19, 2020
Merged

upgrade JSDOM to 15.2.1 #12704

merged 1 commit into from
May 19, 2020

Conversation

erikphansen
Copy link
Contributor

@erikphansen erikphansen commented May 18, 2020

Description

This PR updates JSDOM from 11.1 to 15.2.1

Why?

I was experimenting with React Testing Library and ran into some issues that were due to having such an old version of JSDOM. This upgrade allows us to use React Testing Library if we choose to. It's also a good idea to keep our deps from getting too stale.

Still to do

It would be ideal to upgrade to v16, which is the latest. I ran into significant problems (aXe check failures, lots of component test failures) with the jump from 15 to 16 so will put that off until a later date.

Testing done

All existing tests pass after making a few minor adjustments to our global setup function and a few changes to specific tests.

Screenshots

Acceptance criteria

  • [ ]

Definition of done

  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

@erikphansen erikphansen self-assigned this May 18, 2020
@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 18, 2020 16:20 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 18, 2020 16:20 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 18, 2020 16:30 Inactive
@@ -30,14 +32,20 @@ describe('<MhvTermsAndConditions>', () => {
};

const setup = () => {
global.window.location.replace = sinon.spy();
oldLocation = global.window.location;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes to JSDOM prevent us from directly overriding the location object's properties so we need to delete it and replace it for our testing purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could set window.location to writeable:true similar to what you are doing below and what's mentioned in jestjs/jest#890?

@@ -63,7 +63,18 @@ export default function setupJSDom() {

win.dataLayer = [];
win.scrollTo = () => {};
win.sessionStorage = {};
Object.defineProperty(win, 'sessionStorage', {
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 sessionStorage and localStorage props that JSDOM creates are now readonly which creates issues for some of our tests that try to replace them. This override fixes that.

@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 18, 2020 17:18 Inactive
@erikphansen
Copy link
Contributor Author

erikphansen commented May 18, 2020

This is currently failing in Jenkins due to excess RAM usage and/or a memory leak that seems to stem from JSDOM (which makes sense since that's the substantial change in this PR). The stack traces vary across the the failures. I'll see if I can figure out what's going on and fix it.

image

Failure 1
+ cd /application
+ node script/prearchive.js --buildtype=vagovdev

<--- Last few GCs --->

[16:0x4136f70]    26417 ms: Scavenge 1373.9 (1423.7) -> 1373.1 (1424.2) MB, 1.9 / 0.0 ms  (average mu = 0.139, current mu = 0.083) allocation failure 
[16:0x4136f70]    26424 ms: Scavenge 1374.0 (1424.2) -> 1373.2 (1424.2) MB, 2.2 / 0.0 ms  (average mu = 0.139, current mu = 0.083) allocation failure 
[16:0x4136f70]    26430 ms: Scavenge 1373.8 (1424.2) -> 1373.5 (1425.2) MB, 1.9 / 0.0 ms  (average mu = 0.139, current mu = 0.083) allocation failure 


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x2bf3953dbe1d]
Security context: 0x189ca619e6c1 <JSObject>
    1: next [0x9017e96ba9] [/application/node_modules/symbol-tree/lib/TreeIterator.js:~16] [pc=0x2bf3958fd2ac](this=0x10e4fc284499 <TreeIterator map = 0x48806e96ec1>)
    2: _remove [0x109fcb60a989] [/application/node_modules/jsdom/lib/jsdom/living/nodes/Node-impl.js:922] [bytecode=0x1140f76f40a1 offset=232](this=0x1e0d7c1e0ba9 <EventTargetImpl map = 0x1c0fe...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x8fb090 node::Abort() [node]
 2: 0x8fb0dc  [node]
 3: 0xb031ce v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xb03404 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xef7462  [node]
 6: 0xef7568 v8::internal::Heap::CheckIneffectiveMarkCompact(unsigned long, double) [node]
 7: 0xf03642 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [node]
 8: 0xf03f74 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 9: 0xf06be1 v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [node]
10: 0xed0064 v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [node]
11: 0x11701ee v8::internal::Runtime_AllocateInNewSpace(int, v8::internal::Object**, v8::internal::Isolate*) [node]
12: 0x2bf3953dbe1d 
Aborted

script returned exit code 134
Failure 2
+ cd /application
+ node script/prearchive.js --buildtype=vagovstaging

<--- Last few GCs --->

[16:0x274cf70]    24394 ms: Mark-sweep 1380.2 (1448.2) -> 1368.2 (1450.7) MB, 445.9 / 0.0 ms  (average mu = 0.167, current mu = 0.063) allocation failure scavenge might not succeed
[16:0x274cf70]    24881 ms: Mark-sweep 1381.9 (1450.7) -> 1371.3 (1452.2) MB, 461.0 / 0.0 ms  (average mu = 0.114, current mu = 0.054) allocation failure scavenge might not succeed


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x231aaf1dbe1d]
Security context: 0x13145339e6c1 <JSObject>
    1: queueMutationRecord(aka queueMutationRecord) [0x388defb6b8a1] [/application/node_modules/jsdom/lib/jsdom/living/helpers/mutation-observers.js:~33] [pc=0x231aaf24ea76](this=0x238f38e026f1 <undefined>,type=0x2012303307c1 <String[13]: characterData>,target=0x2155cb451c91 <EventTargetImpl map = 0x18ba5b81dcc1>,name=0x238f38e022b1 <null>,namespace=0x238f38...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x8fb090 node::Abort() [node]
 2: 0x8fb0dc  [node]
 3: 0xb031ce v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xb03404 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xef7462  [node]
 6: 0xef7568 v8::internal::Heap::CheckIneffectiveMarkCompact(unsigned long, double) [node]
 7: 0xf03642 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [node]
 8: 0xf03f74 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 9: 0xf06be1 v8::internal::Heap::AllocateRawWithRetryOrFail(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [node]
10: 0xed0064 v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [node]
11: 0x11701ee v8::internal::Runtime_AllocateInNewSpace(int, v8::internal::Object**, v8::internal::Isolate*) [node]
12: 0x231aaf1dbe1d 
Aborted

@erikphansen erikphansen marked this pull request as ready for review May 18, 2020 21:33
@erikphansen erikphansen requested a review from a team May 18, 2020 21:33
@erikphansen erikphansen requested a review from a team as a code owner May 18, 2020 21:33
@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 19, 2020 19:12 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/eph-jsdom-upgrade May 19, 2020 20:02 Inactive
@@ -253,7 +253,7 @@ def buildAll(String ref, dockerContainer, Boolean contentOnlyBuild) {

def prearchive(dockerContainer, envName) {
dockerContainer.inside(DOCKER_ARGS) {
sh "cd /application && node script/prearchive.js --buildtype=${envName}"
sh "cd /application && node --max-old-space-size=4096 script/prearchive.js --buildtype=${envName}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This step was failing in Jenkins, as described here. Bumping the RAM allowance the same way we do for our unit tests, makes things happy.

@@ -180,7 +180,7 @@
"isomorphic-fetch": "^2.2.1",
"jest": "^23.6.0",
"jest-image-snapshot": "^2.7.0",
"jsdom": "^11.1.0",
"jsdom": "^15.2.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I was trying to accomplish with this PR. All other changes are to adapt to changes/problems introduced by this upgrade. Mostly just changing how certain document properties are mocked so that we can override and spy on them in tests.

@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 19, 2020 20:34 Inactive
@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 19, 2020 20:34 Inactive
@erikphansen
Copy link
Contributor Author

erikphansen commented May 19, 2020

@sanlouise made the change you suggested. seems much better! the diff is a lot smaller at least!

Update: trying to reset window.location globally did not work so I'm sticking with doing it as needed in the tests that need to do it.

@@ -63,7 +63,15 @@ export default function setupJSDom() {

win.dataLayer = [];
win.scrollTo = () => {};
win.sessionStorage = {};
Object.defineProperty(win, 'localStorage', {

Choose a reason for hiding this comment

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

Why do we need to use defineProperty here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason we need to do it for sessionStorage. Changes to JSDOM prevent us from manipulating that property unless we reset it to something we can overwrite which is something we do in some of our tests.

- bump RAM allowance for prearchive scripts in Jenkins
@va-bot va-bot temporarily deployed to vetsgov-pr-12704 May 19, 2020 21:25 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/eph-jsdom-upgrade May 19, 2020 21:49 Inactive
@erikphansen erikphansen merged commit e9ba0f9 into master May 19, 2020
@erikphansen erikphansen deleted the eph-jsdom-upgrade branch May 19, 2020 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants