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

feat(ci): removed the matrix option from unit testing #6394

Merged
merged 3 commits into from
May 17, 2023

Conversation

sethkfman
Copy link
Contributor

@sethkfman sethkfman commented May 15, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Remove the matrix set of unit tests to allow for a single code coverage file generation. The CI run time is approx. +/- 2 min the same as the existing matrix.

Screenshots/Recordings

NA

Issue

NA

Checklist

  • [NA] There is a related GitHub issue
  • [NA] Tests are included if applicable
  • [NA] Any added code is fully documented

@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@sethkfman sethkfman changed the title removed the matrix option from unit testing feat(ci): removed the matrix option from unit testing May 15, 2023
@sethkfman sethkfman marked this pull request as ready for review May 15, 2023 22:53
@sethkfman sethkfman requested a review from a team as a code owner May 15, 2023 22:53
@sethkfman sethkfman added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. labels May 15, 2023
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM.

@Cal-L Cal-L removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 16, 2023
Copy link
Contributor

@leotm leotm left a comment

Choose a reason for hiding this comment

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

this means the components-ui units no longer run ./scripts/run-component-tests.js right
non-matrix CI's passing anyhow, but doesn't look like GH actions expose memory usage to confirm

as an aside locally test:unit with jest ./app/components/UI/ only hitting 2.7GB ram max now for a node process 🎉 so we might be able to revert the temp fix before Jest v29 cc @Gudahtt

@sethkfman
Copy link
Contributor Author

@leotm The updated action is actually running: "test:unit": "jest ./app/ ./locales/", which runs all the tests in ./app. Let me know if I am missing something in your response. Thx

@leotm
Copy link
Contributor

leotm commented May 17, 2023

@leotm The updated action is actually running: "test:unit": "jest ./app/ ./locales/", which runs all the tests in ./app. Let me know if I am missing something in your response. Thx

yep nth missing and updated GH action runs that ^ coverage remains the same 👍

but CI no longer runs "test:unit:components-ui": "./scripts/run-component-tests.js" to fix memory leak on ./app/components/UI/ tests, afaik this is ok (the node process was hitting 23 GB only locally, then net freeze)
just observing seems resolved (2.7 GB) now locally, so we/i can revert the temp fix script pre Jest 29 if agreed
i.e. a chance to clean up tech debt earlier than we expected

@Gudahtt
Copy link
Member

Gudahtt commented May 17, 2023

Interesting. So I guess whatever was causing the explosion in memory usage has been fixed. I wonder what the culprit was.

Perhaps the known Jest memory leak that has no solution until v29 was just a small part of the problem, and the bigger issue was something else entirely.

@Gudahtt
Copy link
Member

Gudahtt commented May 17, 2023

Either way, this seems to work, so I guess we're good for now! We can reduce memory usage further with Jest v29 as we had planned.

Nit: is there any point in keeping around the subdivided unit test scripts, and the Bash script I wrote for the component tests? May as well delete them, I doubt they'll be much use locally.

@sethkfman
Copy link
Contributor Author

@Gudahtt I was going to remove the script in another PR.

The memory leak was resolved in another PR. We are trying to keep changes small.

@sethkfman sethkfman merged commit 764ea2f into main May 17, 2023
10 checks passed
@sethkfman sethkfman deleted the update/unit-testing-ci branch May 17, 2023 19:44
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed/E2E Only Apply this label when your PR does not need any QA effort.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants