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

Setup CI infra to run DevTools tests against multiple React versions #19371

Closed
bvaughn opened this issue Jul 15, 2020 · 42 comments
Closed

Setup CI infra to run DevTools tests against multiple React versions #19371

bvaughn opened this issue Jul 15, 2020 · 42 comments

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jul 15, 2020

PR #19108 caused some Suspense-related DevTools regressions (more info available on #19368) which we did not catch because of the fact that DevTools tests are only run against the version of React in master.

We should follow the precedent of the regression fixtures tests and have CI run DevTools tests against multiple React versions, including v15, all v16 minors, and the current HEAD of master branch.

Setting this up will involve several things:

  • Infra to checkout older React packages and run tests against them.
  • Some form of gating so that we can account for expected differences in Store snapshots between React versions.
  • Some form of gating so that we can avoid running tests against invalid combinations of features and versions (e.g. don't test for Suspense in a version of React that didn't include that component yet).

Which React versions should we test?

In addition to testing against the latest React version in source, I propose that we should also test every minor version going back as far as we support (e.g. starting with v15). We could automate this process like so:

const {exec} = require('child_process');
const semver = require('semver');

const versions = {};

exec('npm view react versions --json', (error, stdout, stderr) => {
  if (stdout) {
    const json = JSON.parse(stdout);
    json.forEach(versionString => {
      if (semver.valid(versionString)) {
        if (semver.gte(versionString, '15.0.0')) {
          const {major, minor, patch} = semver.parse(versionString);

          // Filter out RCs and CI-published daily releases.
          if (`${major}.${minor}.${patch}` === versionString) {
            // Store the last patch for each minor.
            // This relies on the view command returning a sorted releases list.
            const key = `${major}.${minor}`;

            versions[key] = versionString;
          }
        }
      }
    });

    // The "versions" object now contains all versions we should test again.
  }
});
@RitikPandey1
Copy link

I would like to take this as my first issue , please guide me from where I can start

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 21, 2020

@RitikPandey1 You are welcome to work on this issue if you would be interested!

Unfortunately, it would have to be self-guided work. I don't have any more of an outline for what needs to be done than what is written in the issue.

@RitikPandey1
Copy link

Okay, @bvaughn I will try to figure this out .

@MoSattler
Copy link

MoSattler commented Sep 26, 2020

@bvaughn, not sure if this is easily done with checking out older code.
I tried it here, and I keep running into issues where code is either coupled to newer dependencies, or the newer tests are coupled to the newer versions of the code to be tested. See test results here.

Another issue is, that older checkouts have too restrictive devEngines requirements to be installed with modern node, see this.

@MoSattler
Copy link

MoSattler commented Sep 26, 2020

Also tried it by just installing the old dependencies via npm, with similar results. Maybe some of the tests are testing internals, and hence are coupled to internals of the latest code?

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 28, 2020

The build-for-devtools command builds an experimental build (RELEASE_CHANNEL=experimental) of React to be bundled along with the DevTools extension. Generally this build always comes from the master branch, so DevTools is built with the latest version of React.

This task is about using the DevTools to connect to applications written with multiple (usually older) versions of React. Replacing the build-for-devtools command doesn't seem like the right approach here, since that would also be changing which version of React the DevTools is actually running with.

The precedent here would be our "legacy" tests:
https://github.com/facebook/react/tree/master/packages/react-devtools-shared/src/__tests__/legacy

These tests using NPM aliasing to run an older version of React with DevTools (which is still built with the latest experimental build):

// Redirect all React/ReactDOM requires to the v15 UMD.
// We use the UMD because Jest doesn't enable us to mock deep imports (e.g. "react/lib/Something").
jest.mock('react', () => jest.requireActual('react-15/dist/react.js'));
jest.mock('react-dom', () =>
jest.requireActual('react-dom-15/dist/react-dom.js'),
);
React = require('react');
ReactDOM = require('react-dom');

@MoSattler
Copy link

MoSattler commented Sep 30, 2020

Thanks for the elaboration, I will check it out!

@ktfth
Copy link

ktfth commented Mar 30, 2021

@MoSattler can i help you with that task?

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 30, 2021

This task is still up for grabs, if you're interested, @ktfth

@ktfth
Copy link

ktfth commented Mar 30, 2021

@bvaughn I'm totally interested on that task, can you update me with the needs to complete them?

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 30, 2021

Please feel free to work on it then!

Unfortunately other than my comment above (#19371 (comment)) this is a self-guided task. I don't have the bandwidth to provide much support for it.

@ktfth
Copy link

ktfth commented Mar 30, 2021

I'm gonna work to get all information needed. Thank you

@ktfth
Copy link

ktfth commented Mar 30, 2021

@bvaughn I have some question after investigate the process mentioned on #19371 (comment), the test need to be runned just on legacy code and for each version of react?

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 31, 2021

I don't know what you mean by "just legacy code"

Ideally probably at least each minor version of React

@ktfth
Copy link

ktfth commented Mar 31, 2021

The legacy test code showed by you on comment, do you remember?

@ktfth
Copy link

ktfth commented Mar 31, 2021

@bvaughn Maybe is not clear what is need to do on that task, can elaborate more about the case is need to cover with the setup?

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 31, 2021

Was referring to my comment above: #19371 (comment)

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 31, 2021

I don't have a lot of time to go into much detail here, but the idea is that we currently run automated DevTools tests against two flavors of React:

  1. The latest (head of master branch)
  2. The latest release of React 15 (legacy tests)

Ideally we would also run against every minor release of 16. There are a couple of challenges here:

  1. How to install and run the tests against each release
  2. How to gate which tests don't apply to all v16 releases (e.g. b'c it's a feature not added until a later version).

@ktfth
Copy link

ktfth commented Mar 31, 2021

@bvaughn Great this clarify the work, thank you

@ktfth
Copy link

ktfth commented Mar 31, 2021

I have made some tests here to create a case on CI there is the result first change, second change and the pipeline

@ktfth
Copy link

ktfth commented Mar 31, 2021

I'm gonna update here to you @bvaughn can review my changes and get the job done

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 31, 2021

Okay. Keep me posted when your PR is ready for a look 👍🏼

@ktfth
Copy link

ktfth commented Mar 31, 2021

Now i need to reproduce the same tests of legacy for other versions like 16 for every minor?

@ktfth
Copy link

ktfth commented Mar 31, 2021

Sorry about a lot of questions, but you have some example to give on that part?

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 1, 2021

Sorry about a lot of questions, but you have some example to give on that part?

No, sorry.

@ktfth
Copy link

ktfth commented Apr 1, 2021

Some updates on config based on @MoSattler work to test every version of react with the result on ci. The limits of service was reached. @bvaughn how can we continue?

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 1, 2021

Sorry @ktfth, but I don't have much bandwidth to support this task. Unfortunately it has to be self-guided 😦 I'll make time to review a PR but I don't think I can be involved in the troubleshooting phase at the moment.

@ktfth
Copy link

ktfth commented Apr 1, 2021

@bvaughn I'm gonna make a PR, thank you so much

@ktfth
Copy link

ktfth commented Apr 1, 2021

@bvaughn The PR is #21166 for this issue

@hardikshah197
Copy link

hardikshah197 commented May 4, 2021

Can I take up this issue? If it isn't fixed up yet @bvaughn

@bvaughn
Copy link
Contributor Author

bvaughn commented May 4, 2021

@hardikshah197 If you'd like to take a look at #21166 and maybe lend a hand?

@hardikshah197
Copy link

Sure, I'll give it a look on and try it out. Thanks

@Albert-Jokelin
Copy link

@bvaughn is #21166 still open?

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 19, 2021

Yes.

@Albert-Jokelin
Copy link

I want to work on this.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 19, 2021

This issue is all yours, @Albert-Jokelin! 😄

I've added the "good first issue (taken)" label so that others will know not to start work on the issue. If you change your mind about the issue, no worries! Just let me know so that I can remove the label and free it up for someone else to claim.

Cheers!

@streetcredmusicgroup
Copy link

streetcredmusicgroup commented Aug 19, 2021 via email

@Albert-Jokelin
Copy link

@bvaughn My account with CircleCI doesn't allow me to test the pipeline completely (get stuck at the paywall), is there anyway where I could use React's pipeline?

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 24, 2021

@Albert-Jokelin Creating a PR on this repo (even just a Draft PR) should enable that, no?

@Albert-Jokelin
Copy link

Nope, it isn't working

Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Copy link

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants