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

tools: fix Galaxy discovery #8709

Merged
merged 5 commits into from May 21, 2017

Conversation

Projects
None yet
2 participants
@glasser
Member

glasser commented May 20, 2017

1a03655 in 1.4.4.2 expanded on the HTTP error checking added by 30aec9f in
1.4.2. Neither of these changes were aware that discoverGalaxy invokes
httpHelpers.request with json:true, resulting in a body that is a parsed JSON
object rather than a string or Buffer. Before 1.4.4.2, this had no consequences
because body.length is undefined and undefined < 90 is false, but the change
to Buffer.byteLength actually made the condition true.

It's safe to not check length in the JSON case because a truncated JSON object
is not legal JSON (unless the truncation just drops trailing whitespace, in
white case that's OK).

I check for both string and Buffer because some calls to this function pass in
an encoding option. Buffer.byteLength works with both types.

tools: fix Galaxy discovery
1a03655 in 1.4.4.2 expanded on the HTTP error checking added by 30aec9f in
1.4.2. Neither of these changes were aware that discoverGalaxy invokes
httpHelpers.request with json:true, resulting in a `body` that is a parsed JSON
object rather than a string or Buffer.  Before 1.4.4.2, this had no consequences
because body.length is undefined and `undefined < 90` is false, but the change
to Buffer.byteLength actually made the condition true.

It's safe to not check length in the JSON case because a truncated JSON object
is not legal JSON (unless the truncation just drops trailing whitespace, in
white case that's OK).

I check for both string and Buffer because some calls to this function pass in
an encoding option.  Buffer.byteLength works with both types.

@glasser glasser requested review from benjamn and abernix May 20, 2017

Style tweaks and a small bug fix.
These checks should still happen when body is an empty string, which (for
better or worse) is falsy in JavaScript.

@benjamn benjamn added this to the Release 1.4.4.3 milestone May 21, 2017

@benjamn benjamn self-assigned this May 21, 2017

benjamn added some commits May 15, 2017

Re-run individual tests to avoid re-running the whole suite.
To deal with individual flaky tests, we often just re-run the entire test
suite, which feels like an enormous waste of shared computing resources.

This change automatically re-runs individual failed tests as many as two
more times, and considers the test successful if any of those attempts
succeeds.

cc @abernix @hwillson et al.
Call requestGarbageCollection in Isopack#_writeTool.
This method appears to be causing large spikes in memory consumption on
Circle CI during the `meteor --get-ready` preparation step, which often
leads to the test process being killed.

Also added a call in IsopackCache#_loadLocalPackage for good measure.

We're now calling requestGarbageCollection pretty frequently when
we run Node with --expose-gc, but that currently only happens during
Circle CI tests, so I don't think we need to implement the improvements
suggested in tools/utils/gc.js, yet.

Previously: 35f488e, f6df21f
Try not running a full meteor --get-ready before Circle CI tests.
In the ongoing struggle with Circle CI-specific test failures, the
preparatory `meteor --get-ready` has been a consistent point of failure,
before any real tests have the chance to run.

Using a lighter-weight command (meteor --help) that still does most of
what --get-ready did seems worth a try, though it might just defer
memory-intensive work until later, so we'll have to see what happens.

@benjamn benjamn merged commit fb37811 into devel May 21, 2017

4 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

benjamn added a commit that referenced this pull request May 22, 2017

@benjamn benjamn referenced this pull request May 22, 2017

Merged

Release 1.4.4.3 #8711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment