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

Update versions of all the things used in tests #2242

Merged
merged 68 commits into from Feb 26, 2019
Merged

Update versions of all the things used in tests #2242

merged 68 commits into from Feb 26, 2019

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Jan 30, 2019

You can no longer build modules for node 6 on macOS 10.14, so we need to upgrade to a newer version. Unsurprisingly this then has cascading effects where everything else needs to be upgraded too.

An overview of the things changed here:

  • Node 8 and npm 6 are now used for running all the test scripts. install-local is used to mimick the pre-npm5 behavior when installing modules from filepaths where needed (i.e. where symlinking in the realm module breaks things).
  • React Native is upgraded to 0.58. This requires switching to use CocoaPods for building the native modules on iOS, which also removes the need for some of our hacks. The react native iOS tests now actually work (7e8b7d7 broke the test app and it's been crashing when running on CI for months without reporting a failure).
  • Updated Android stuff to work with newer versions of gradle. There's still some deprecation warnings that indicate it'll break again with gradle 6.
  • Upgraded eslint to 5.0 and fixed a pile of warnings and errors from the new recommended setting.
  • Bumped the electron version used for tests to 4.0 and fixed some unrelated pre-existing issues with running the tests in electron.
  • Fixed some assorted minor issues when building with Xcode 10 and bumped the last-upgrade-check versions.
  • Update to the newest versions of core/sync/ROS. The react-native tests were hitting an assertion failure in sync fixed in 3.15.0.
  • Chrome debugger/RPC server tests are now actually run (with recent versions of Xcode the Chrome_ tests failed to set the executor at the right time), and a pile of broken things in that are fixed.

@jgreen01su
Copy link

Is there anything I can do to help? I need iOS support for RN 0.58.x.

@nonewcode
Copy link

Any status update on this PR?

@tgoyne tgoyne force-pushed the tg/npm-5 branch 3 times, most recently from 189d049 to 1943428 Compare February 16, 2019 00:02
@kerryaustin
Copy link

Any update on this PR? Any way I can help?

@bmunkholm bmunkholm requested review from kneth, nirinchev, cmelchior and kraenhansen and removed request for nirinchev February 20, 2019 06:56
@bmunkholm
Copy link
Contributor

Left is just to review.

examples/ReactExample/package.json Outdated Show resolved Hide resolved
react-native/ios/RealmReact/RealmReact.mm Show resolved Hide resolved
react-native/package.json Outdated Show resolved Hide resolved
response.body.pipe(file).once('finish', () => file.close(resolve));
}).then(() => fs.utimes(destination, lastModified, lastModified));
}
}).catch(() => saveFile(response));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we saving the file in case of an error?

Copy link
Member

Choose a reason for hiding this comment

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

This will hide the error, is that on purpose? I would console.error it at the least.

Copy link
Member Author

Choose a reason for hiding this comment

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

fs.stat() throws if the file does not exist or if the user doesn't have permission to access it. The former case isn't an error and we just want to download the file, and in the latter case saveFile will throw an error anyway.

tests/js/realm-tests.js Outdated Show resolved Hide resolved
"babel-jest": "18.0.0",
"babel-preset-react-native": "4.0.0",
"eslint": "^3.0.0",
"install-local": "^0.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

Looks like 1.0.0 shipped 8 days ago.

As this is an example I feel it's a little misleading to install Realm JS using this package. People might be looking at it as a reference on how to install Realm JS and get confused. The integration test I just merged had the same issue and I solved it by packing Realm JS and installing it from the package install script like so: https://github.com/realm/realm-js/blob/master/integration-tests/environments/react-native/package.json#L10 .. I don't know if that will be less confusing - but I wanted to mention an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Manually acquiring a tarball and calling npm install on that is significantly more dissimilar to how end users should install realm than this is. The point of using install-local is that it makes it almost identical to a normal dependency other than that it's in a separate section in package.json.

Copy link
Member

Choose a reason for hiding this comment

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

There is also a case to be made that this will actually not be using the .tgz that will eventually ship to users and instead it will package its own. Installing from the .tgz will catch more errors from potential issues with the way we might choose to package the archive.

Copy link
Member Author

Choose a reason for hiding this comment

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

install-local just calls npm pack and installs from that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason we’ve had issues with regressions not being caught by tests in the past isn’t due to minor details around how realm is installed in tests: it’s because the node release is built from realm-js-private with a different build system and some important code differences, and while rjp has stuff set up to run the public tests against that module, it’s been broken for years. Assembling a tarball from this repo and installing from that isn’t any closer to testing the thing that users will actually install than anything else these tests have been doing.

Copy link
Member

Choose a reason for hiding this comment

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

Building from the package that we've already build at this point might speed up installing this app. I don't have a strong opinion here, thus it's not a blocker from my perspective - just wanted to share my thoughts.

Copy link
Member

@kneth kneth left a comment

Choose a reason for hiding this comment

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

Great work - much appreciated!

I need to go through it once more.

Jenkinsfile Outdated

def packageNpmArchive() {
return {
node('docker') {
Copy link
Member

Choose a reason for hiding this comment

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

Adding && !aws has been a help in the past.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that will do much, we have just a single node with that label https://ci.realm.io/label/aws/

Copy link
Member

Choose a reason for hiding this comment

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

It looks like some aws nodes are spawned from time to time (probably part of our cloud infrastructure).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, Jenkins autoscales a set of slaves using spot instances.

scripts/download-realm.js Outdated Show resolved Hide resolved
getContext() is not visible in extensions.js so _createUserAgentDescription()
would always just throw an exception and report RealmJS/Unknown.
@HarikaSabbella
Copy link

@kneth when can we expect these changes to be released to NPM?

@bmunkholm
Copy link
Contributor

@HarikaSabbella When we are done with the review - can't really give a date sorry, but likely sometime next week hopefully. Please have patience so we get the quality right. The beauty of open source is that if you can't wait, you can grab what is here if that works for you ;-)

@munsifhayat
Copy link

@bmunkholm Waiting passionately.

Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

Great work! 🎉

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: tgoyne <tgoyne@gmail.com>
@tgoyne tgoyne merged commit 003f8b3 into master Feb 26, 2019
@tgoyne tgoyne deleted the tg/npm-5 branch February 26, 2019 19:36
@diegolmello
Copy link

Nice! Is this going to be released today? 😆

@nonewcode
Copy link

nonewcode commented Feb 26, 2019 via email

@kneth
Copy link
Member

kneth commented Feb 27, 2019

@diegolmello I am not sure if we can make it today. If not, we'll get it out tomorrow.

@kneth
Copy link
Member

kneth commented Feb 27, 2019

@diegolmello I did have some time to get it released 😄

@diegolmello
Copy link

@kneth Hahahahaha. Great work, man. I'll try it later today. Thanks! 👊👊👊

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet