fix(CI): prevent the e2e test from running on 0.XX-stable branches

[kelset]

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)

Once merged, we can cherry-pick in 0.71-stable.

Changelog

[INTERNAL] [FIXED] - prevent the e2e test from running on 0.XX-stable branches

Test Plan

I used this PR for testing: #35665

here's a screenshot of when things work (no E2E command run):

here's how it looks when they don't (E2E runs and crashes):

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  
    }
}

[facebook-github-bot]

@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 1b7127b
Branch: main

[cipolleschi]

Doesn't this mean that we don't run JS tests at all in the stable branch? 🤔

If that's the case, what about changing the e2e script so that, if there is a workspace entry in the package.json, we use that. Otherwise we can pass the path to the packages folder and the script can consider all the folder in that path as packages to try and publish.

By doing that, Verdaccio will work as if it was a workspace and we could retain JS test on stable as well.

WDYT?

[kelset]

Doesn't this mean that we don't run JS tests at all in the stable branch? 🤔

No, it's only a logical condition for run_e2e which is here https://github.com/facebook/react-native/blob/main/.circleci/config.yml#L259 and that corresponds to https://github.com/facebook/react-native/blob/main/scripts/run-ci-e2e-tests.js which is a script that we should completely rework since the "E2E" part of it is some old smelly code that basically doesn't do anything useful. But for some reason the Verdaccio part was added here so we can't just entirely remove this script and CircleCI command.

You can also see it in the screenshot, that the normal Javascript tests run and are green.

[cipolleschi]

Ok, so we skip only the e2e test which are not providing value, right now.
Then, that's fine.

[cortinico]

that runs verdaccio and verdaccio needs the workspace, but the workspace is not there because of the aforementioned ✨dark magic✨

😢

[NickGerleman]

since the "E2E" part of it is some old smelly code that basically doesn't do anything useful. But for some reason the Verdaccio part was added here so we can't just entirely remove this script and CircleCI command.

So, the e2e part is kind of misleading, but the unique bit about this script is that it runs tests against a newly initialized app. E.g. it is where we test that a new app typechecks.

[NickGerleman]

The reason Verdaccio is used here is so that tests can be run simulating a real installation from an npm registry. There is a seprate bit using file copies, but it does some surprising stuff which can cause some tests to hang because of differences compared to a real app init.

[NickGerleman]

TLDR: The script does provide some unique coverage. So, I am a little weary of removing this, unless it is needed to unblock the release (then maybe we could just disable in the stable branch)?

[cortinico]

then maybe we could just disable in the stable branch

That's essentially what the PR is doing right?

[NickGerleman]

then maybe we could just disable in the stable branch

That's essentially what the PR is doing right?

Sorry, I meant checking the change to disable only into the stable branch, then working to fix the underlying issue on main. That way we can remove the coverage specifically for 0.71-stable, but prevent us from accidentally bringing the debt forward to future stable branches.

[kelset]

ok so just to be clear, y'all basically just suggesting to "retarget" this PR so that it's local to 0.71 and then in main the work will be tackled differently?

To be clear, the verdaccio stuff will never work on -stable branches because we remove the workspace section of the root package.json, and we won't be able to change that until we finish all the monorepo work I think. This is why I'm doing the commit against main branch, I reckon that this "workaround" will need to be in place for 0.72 too at least.

[hoxyq]

I wonder if my latest change might solve this problem?

If this occurs when we execute yarn workspaces --info, then it was replaced in 7f29357

Now we can iterate through each package inside /packages/* without yarn workspaces command

[kelset]

I wonder if my latest change might solve this problem?

If this occurs when we execute yarn workspaces --info, then it was replaced in 7f29357

could be, actually! Should we try to cherry pick this into 0.71-stable, push up the commit and see if CI is green? if that's the case we could close this PR off...

[kelset]

Update on all of this - I've spent a bit more time and I wanted to share the learnings:

• in main branch, we don't need this change because of @hoxyq's work. Great stuff there. I'm going to close this PR then.
• For 0.71, we have 3 alternatives:
  1. we don't do anything (good enough, but some small JS/typing issues might go undetected)
  2. we merge a variation of this PR, I've made that PR here: fix(CI): prevent the e2e test from running on 0.71 #35681
  3. we figure out a way to port @hoxyq's work in 0.71-stable. I did a rough attempt to see what would happen here: fix(CI): cherry-picking changes to logic for fixing JS CI tests [TEST] #35680 the main issue is that we can't cherry-pick cleanly 7f29357 but it needs the other commits from him, and in particular 83afdaf which I'm pretty sure we don't want/can't have in the 0.71 branch since it's pre-all the refactorings and renaming happening in main. So it'd basically have to be a proper PR for 0.71 only, so might not be worth pursuing.

We can pick this up in Jan btw, and fix for 0.71.1, no rush.