Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: mfs preload test #1551

Merged
merged 1 commit into from
Sep 12, 2018
Merged

fix: mfs preload test #1551

merged 1 commit into from
Sep 12, 2018

Conversation

alanshaw
Copy link
Member

JS makes no guarantees a function you schedule with setTimeout will execute before another function you schedule even if the second function is scheduled for execution after the first.

This PR fixes the MFS preload test to wait for the expected CIDs to have been preloaded instead of assuming they would be preloaded after a certain time.

@ghost ghost assigned alanshaw Sep 10, 2018
@ghost ghost added the status/in-progress In progress label Sep 10, 2018
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

JS makes no guarantees a function you schedule with `setTimeout` will execute before another function you schedule _even_ if the second function is scheduled for execution _after_ the first.

This PR fixes the MFS preload test to wait for the expected CIDs to have been preloaded instead of assuming they would be preloaded after a certain time.

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
}

waitFor(test, { name: 'CIDs to be preloaded' }, (err) => {
expect(err).to.not.exist()
Copy link
Member

Choose a reason for hiding this comment

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

rather than expect(err).to.not.exist() we should be using if (err) return done(err) so we properly finish off the test when it errors

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean so that we always call preloader.stop?

I'm not sure I understand why using done(err) is necessary in this case and not elsewhere in the tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanshaw
Copy link
Member Author

Ok I'm going to merge this as for now so that we can get other PRs passing. We can fix/update later if needs be. All green except one CI failure, which seems to be a CI issue not an issue with this code.

@alanshaw alanshaw merged commit 7c7a5a6 into master Sep 12, 2018
@ghost ghost removed the status/in-progress In progress label Sep 12, 2018
@alanshaw alanshaw deleted the fix/preload-test branch September 12, 2018 12:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants