Skip to content

Conversation

@iamstarkov
Copy link

@iamstarkov iamstarkov commented Jan 23, 2020

CI=true check is insufficient.

It doesnt work for Jenkins, Teamcity, TaskCluster. Where Jenkins and Teamcity are big ones.

If you dont want to bring is-ci and ci-info in, I understand, but let me adjust the PR with this snippet from ci-info:

https://github.com/watson/ci-info/blob/master/index.js#L52-L59

exports.isCI = !!(
  env.CI || // Travis CI, CircleCI, Cirrus CI, Gitlab CI, Appveyor, CodeShip, dsari
  env.CONTINUOUS_INTEGRATION || // Travis CI, Cirrus CI
  env.BUILD_NUMBER || // Jenkins, TeamCity
  env.RUN_ID || // TaskCluster, dsari
  false
)

Verification: I tried modified fork on a local jenkins and it react-scripts test dont hang up anymore

CI=true check is insufficient.

It doesnt work for Jenkins, Teamcity, TaskCluster. Where Jenkins and Teamcity are big ones.
@facebook-github-bot
Copy link

Hi iamstarkov! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@iamstarkov
Copy link
Author

build fails in some environments due to some unrelated tests failing

FAIL fixtures/issue-5176-flow-class-properties/index.test.js (24.932s)
  ● passes tests

    expect(received).toBe(expected) // Object.is equality

    Expected: true
    Received: false

      5 |     jestEnvironment: 'node',
      6 |   });
    > 7 |   expect(fulfilled).toBe(true);
        |                     ^
      8 | });
      9 | 

      at Object.<anonymous>.test (fixtures/issue-5176-flow-class-properties/index.test.js:7:21)

https://dev.azure.com/facebook/create-react-app/_build/results?buildId=1029&view=logs&j=60986ac1-7273-5058-049d-3a5eba1546b1&t=bd762266-dd4b-5588-0371-4e3c4f476f1d&l=497

I dont understand why that can be failing, if someone will explain why, I'll fix it

@iamstarkov
Copy link
Author

I figured it out, master is unhealthy and fails in the same environments with similar errors. Therefore my PR didnt break anything more than current master did

@stale
Copy link

stale bot commented Feb 22, 2020

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 22, 2020
@iamstarkov
Copy link
Author

@Stale please chill. its stale, only because nobody reviewed it yet

@stale stale bot removed the stale label Feb 24, 2020
@ianschmitz
Copy link
Contributor

ianschmitz commented Feb 28, 2020

Wouldn't adding the environment variable to your pipeline suffice? That's what most other folks are doing in environments that don't set CI=true by default (but is default in travis, circle, etc)

@iamstarkov
Copy link
Author

it does suffice, but the bug itself is a regression (judging from my experience and by the open issues), so my intention was to fix the regression problem

@iamstarkov
Copy link
Author

we migrated away from Jenkins

@iamstarkov iamstarkov closed this Feb 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants