Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[bug] Stable sort changes for v8 #2497

Merged
merged 3 commits into from
Jun 18, 2020
Merged

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Jun 17, 2020

Description

Array.sort changed from node 10 to node 12. The sort function used here was not resilient to this, so it broke.

https://v8.dev/features/stable-sort
nodejs/node#27871

Related Issue

Closes PWA-690.

Acceptance

Verification Stakeholders

@zetlen

Specification

Verification Steps

  1. nvm install 12
  2. nvm use 12
  3. yarn clean:all
  4. yarn install
  5. yarn test --coverage=0 packages/pwa-buildpack/lib/Utilities/__tests__/loadEnvironment.spec.js

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have updated the documentation accordingly, if necessary.

Signed-off-by: sirugh <rugh@adobe.com>
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 17, 2020

Fails
🚫

node` failed.

🚫

Unit tests in the following files did not pass 😔. All tests must pass before this PR can be merged

  • packages/peregrine/lib/talons/CheckoutPage/__tests__/useCheckoutPage.spec.js
Messages
📖

Associated JIRA tickets: PWA-690.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Log

ERROR ON TASK: unitTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 2893d0f

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jun 17, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2497.pwa-venia.com : LH Performance Expected 0.85 Actual 0.57, LH Best Practices Expected 1 Actual 0.92
https://pr-2497.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.37, LH Best Practices Expected 1 Actual 0.92
https://pr-2497.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.48, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

@supernova-at supernova-at added the version: Patch This changeset includes backwards compatible bug fixes. label Jun 18, 2020
@dpatil-magento
Copy link
Contributor

Updated CI test build node to 12.

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I should have known better than to use that sort predicate before.

@dpatil-magento
Copy link
Contributor

packages/peregrine/lib/talons/CheckoutPage/tests/useCheckoutPage.spec.js already exists on develop with node12. I will create seperate ticket for this.

I have verified this pr on local and on CI with node 10 and 12, looks good.

@dpatil-magento dpatil-magento merged commit bad6921 into develop Jun 18, 2020
@dpatil-magento dpatil-magento deleted the rugh/pwa-690-node12 branch June 18, 2020 22:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:pwa-buildpack version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants