Run unit-tests on GHA #48499
Open
Run unit-tests on GHA #48499
+21
−13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
cipolleschi
requested changes
Jan 6, 2025
Contributor
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for working on this. I see that the CI jobs are red, so this probably need some more work. If they are red because they depend on #48498, you can change the destination branch to be the other PR:
I also left a comment to reduce a bit the CI costs
.github/workflows/test-all.yml
Outdated
| jsengine: ${{ matrix.jsengine }} | ||
| architecture: ${{ matrix.architecture }} | ||
| run-unit-tests: "false" | ||
| run-unit-tests: true |
Contributor
There was a problem hiding this comment.
the problem with this is that we will run unit tests on ALL the matrix configuration. This is a bit of an overkill and it will increase the costs of CI sensibly.
I believe it would be better to copy this job in the workflow and to run unit tests with the Hermes, New Architecture, Static Library configuration only. This should be enough.
Contributor
Author
There was a problem hiding this comment.
copy this job in the workflow
Instead of making a copy, I'd suggest adding this as a parameter to the matrix and use the include feature to run it only on the configurations you mentioned.
Would you be okay with keeping things simple for now and ☝️ to be a separate PR?
Contributor
There was a problem hiding this comment.
can you move this to matrix in this pr?
This is not pretty complex, and adding a param to the matrix should not add a lot of noise.
Contributor
Author
There was a problem hiding this comment.
Hermes, New Architecture, Static Library configuration only
For both Debug and Release or just one of these?
Contributor
Author
I probably could, but then you'd be reviewing a PR in my fork. |
kraenhansen
force-pushed
the
run-unit-tests-on-gha
branch
from
f4e45ed to
cc8efb9
Compare
Contributor
Author
|
I've rebased + force-pushed since the merge of #48498. |
Contributor
Author
|
This is output from the log of the "test_ios_rntester (Hermes, NewArch, Release)" job: |
cipolleschi
requested changes
Jan 16, 2025
.github/workflows/test-all.yml
Outdated
| jsengine: ${{ matrix.jsengine }} | ||
| architecture: ${{ matrix.architecture }} | ||
| run-unit-tests: "false" | ||
| run-unit-tests: true |
Contributor
There was a problem hiding this comment.
can you move this to matrix in this pr?
This is not pretty complex, and adding a param to the matrix should not add a lot of noise.
Contributor
|
if you rebase on main once again, the problem should be fixed. Unit tests needs an emulator to run and I changed the CI to build for any emulator as we want also the RNTester App to be downloadable for other reasons... |
kraenhansen
force-pushed
the
run-unit-tests-on-gha
branch
from
cc8efb9 to
816c705
Compare
Contributor
Author
|
I've rebased and pushed with a commit adding |
Contributor
Author
|
test_ios_rntester (Hermes, NewArch, Release) is failing with an undefined symbols error while linking: |
Contributor
|
Main is green... do you mind rebasing on top of it? |
kraenhansen
force-pushed
the
run-unit-tests-on-gha
branch
from
816c705 to
7b32b83
Compare
Contributor
Author
|
I've rebased on latest |
Contributor
Author
|
The tests are failing now due to this change, introduced four days ago with #48755. I can also include that fix in this PR if you'd want me to? |
Contributor
|
We are making several changes in this infrastructure, so I expect for some back and forth here. :( |
kraenhansen
force-pushed
the
run-unit-tests-on-gha
branch
from
7b32b83 to
608016c
Compare
Contributor
|
Closing as not relevant anymore |
Contributor
Author
|
@cortinico interesting, because the PR is outdated or the code is going away? I still don't see the unit tests being enabled in the test-all workflow on |
Contributor
Ah you're right. I've overlooked this PR as I thought it was related to E2E tests (which we change significantly over the last months), while instead it's related to iOS unit tests, so we can reopen it. |
Collaborator
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
react-native-bot
added
the
Stale
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
As suggested by this comment, #45133 inadvertently disabled unit tests on GHA and I believe these changes will re-enable running them.
To actually make those green again, I believe #48498 needs to merge first.
Changelog:
[INTERNAL] [FIXED] - Start running unit tests on GHA again.
Test Plan:
I'll await the GHA logs 🤞