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

fix(CI): prevent the e2e test from running on 0.71 #35681

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Dec 19, 2022

(new variation of #35667, read the last comment there for more info)

Summary

Because of the usual ✨dark magic✨ that we do 'cause RN is not a proper monorepo (usual link react-native-community/discussions-and-proposals#480), one of the side effects is that test_js and test_js_prev_lts insta-crash whenever they run in a -stable branch because of the run_e2e job, that runs verdaccio and verdaccio needs the workspace, but the workspace is not there because of the aforementioned ✨dark magic✨

This PR fixes that by not allowing said command to be run when we are in 0.XX-stable branches. (TIL that under the hood CircleCI wants Java regex)

In main we don't need it because of the work of @hoxyq (see 7f29357)

Changelog

[INTERNAL] [FIXED] - prevent the e2e test from running on 0.71

Test Plan

I used this PR for testing: #35665

here's a screenshot of when things work (no E2E command run):
Screenshot 2022-12-16 at 14 52 00

here's how it looks when they don't (E2E runs and crashes):
Screenshot 2022-12-16 at 14 58 04

You can also verify that the condition is correct by throwing this sample code in an online Java ide:

import java.util.regex.*;  

public class Main
{
    public static void main(String[] args) {
        System.out.println(Pattern.matches("0.[0-9]{2}-stable", "main"));//false  
        System.out.println(Pattern.matches("0.[0-9]{2}-stable", "0.71-stable"));//true
        System.out.println(Pattern.matches("0.[0-9]{2}-stable", "just-a-test-with-stable"));//false
        System.out.println(Pattern.matches("0.[0-9]{2}-stable", "just-a-test-with-stable-test"));//false  
    }
}

It's expected to fail here because the name of the branch is NOT 0.XX-stable ;)

this should work

typo

needs to be in  a different place

we should only filter the e2e part

no need for this anymore

now it should work

one more time, with gusto

try a fix

it's java pattern matchin

stricter

final touches
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner labels Dec 19, 2022
@kelset kelset changed the base branch from main to 0.71-stable December 19, 2022 11:53
@kelset kelset requested review from cipolleschi, cortinico, NickGerleman and hoxyq and removed request for mhorowitz December 19, 2022 11:53
@ryancat
Copy link
Contributor

ryancat commented Dec 19, 2022

@kelset I saw some CI tests are failing with the same error message:

{"type":"error","data":"Cannot find the root of your workspace - are you sure you're currently in a workspace?"}

Doesn't this change suppose to fix that test?

Comment on lines +536 to +546

# When not in 0.XX-stable branch, run e2e tests
- when:
condition:
not:
matches:
pattern: "0.[0-9]{2}-stable"
value: << pipeline.git.branch >>
steps:
- run_e2e:
platform: js
Copy link
Contributor

@NickGerleman NickGerleman Dec 20, 2022

Choose a reason for hiding this comment

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

Suggested change
# When not in 0.XX-stable branch, run e2e tests
- when:
condition:
not:
matches:
pattern: "0.[0-9]{2}-stable"
value: << pipeline.git.branch >>
steps:
- run_e2e:
platform: js

From @ryancat's comments it looks like the test we were wanting to disable might still be running. This change originally came from the main branch to conditionally run on stable branches.

I wonder if << pipeline.git.branch >> might evaluate to the branch a PR comes from or some jazz like that. But if this is only to the single branch we could also just remove everything.

FYI @ryancat, since this change targets a "stable branch" instead of "main", we do not sync it to Phabricator, and this change can be committed directly in GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickGerleman the test is still running on this PR because the branch is kelset/filter-e2e-on-071 ;)

@cipolleschi
Copy link
Contributor

The failure you see are correct. The pipeline.git.branch you see are happening because the PR is on a branch called kelset/filter... which does not satisfy the RegEx, of course. The tests are run, but the base branch is a stable, so they fail.

I think that the condition is missing a case: we don't want to run those tests if:

  • the CI is running on 0.xx-stable
    or
  • the PR is opened against an 0.xx-stable branch.

WDYT?

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

We can merge this as it is, to cleanup the release workflow when we push a tag, do the cherry-picks and for the actual release jobs. All of these run directly on 0.XX-stable.

A more complete solution, will skip the e2e tests also for PRs opened against the stable branch. But next year we will revamp the E2E testing, so it makes sense to wait for that, probably.

@NickGerleman
Copy link
Contributor

NickGerleman commented Jan 2, 2023

We can merge this as it is, to cleanup the release workflow when we push a tag, do the cherry-picks and for the actual release jobs. All of these run directly on 0.XX-stable.

A more complete solution, will skip the e2e tests also for PRs opened against the stable branch. But next year we will revamp the E2E testing, so it makes sense to wait for that, probably.

There is some helpful context in the comment @kelset links to. These tests are for project initialization (not the UI "e2e" testing they might imply), but also the underlying issue preventing them from running in stable branches has already been fixed in main.

It might not hurt to rename these scripts/tests to something different. The only reason I know what these scripts do is stumbling into them looking for project initialization tests for the TS change 😅.

@kelset
Copy link
Contributor Author

kelset commented Jan 3, 2023

It might not hurt to rename these scripts/tests to something different. The only reason I know what these scripts do is stumbling into them looking for project initialization tests for the TS change 😅.

totally, but again - this PR is just a fix for 0.71-stable branch, in main you can go ahead and do more substantial changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants