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

CircleCI Integration #1479

Merged
merged 12 commits into from
Nov 13, 2017
Merged

CircleCI Integration #1479

merged 12 commits into from
Nov 13, 2017

Conversation

fearphage
Copy link
Member

I'm working on the CircleCI integration to potentially replace TravisCI. So far, the problem I'm having is I don't understand why the webworker test (test-webworker) is failing on Node 6 and 8. FYI it's not run on Node 4. I mostly copied the logic of the travis config into circle.

Notes: I'm using their new fancy 2.0 configuration which is new to me.

@sinonjs/sinon-core Any ideas? I can't reproduce locally and I don't see anything out of the ordinary. Is there a way to get any more feedback from the failure?

@coveralls
Copy link

coveralls commented Jul 3, 2017

Coverage Status

Coverage remained the same at 94.998% when pulling 9c083f8 on fearphage:circleci-integration into 079925b on sinonjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.998% when pulling 9c083f8 on fearphage:circleci-integration into 079925b on sinonjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.998% when pulling 9c083f8 on fearphage:circleci-integration into 079925b on sinonjs:master.

@fatso83
Copy link
Contributor

fatso83 commented Jul 3, 2017

sorry, no idea. this change smells a bit like the YAML vs JSON vs JS issue for me. Personally, I don't really care as long as it works, as it's just too little time saved to care, but remind me again why you are doing this; aren't these things pretty much the same?

Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

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

As @fatso83 commented, are there any special reasons for doing this?

TravisCI has always worked well for me and it has great integration with many services. Have we been having any problems with it lately? Sometimes it could be easier to get in touch with their support and see if these problems can be solved so that we don't have to change infrastructure.

I'm not invalidating these changes, I think you've done great work with all this infrastructure changes, I just wanted to make sure we will avoid unnecessary work.

When it comes to why this test is failing, I've done a quick research and I think it might be because circleCI is trying to do cross-domain XHR. This StackOverflow answer might help you.

@fearphage
Copy link
Member Author

this change smells a bit like the YAML vs JSON vs JS issue for me

It has nothing to do with the config file itself to my knowledge.

TravisCI has always worked well for me and it has great integration with many services. Have we been having any problems with it lately? Sometimes it could be easier to get in touch with their support and see if these problems can be solved so that we don't have to change infrastructure.

I just looked into, because of the problems with running tests in Node 8 on travis. I prefer CircleCI and that's what I've used professionally. I have yet to see an integration that Travis has over Circle. I'm just putting it out there as an option. I also like that it reports each node version individually instead of as a set. Thus you don't have to click through to see where the failures are.

This StackOverflow answer might help you.

Check out the actual build. It's running phantomic with that option:

browserify --no-commondir --full-paths -p [ mocaccino -R spec --color ] test/webworker/webworker-support-assessment.js | phantomic --web-security false

https://circleci.com/gh/fearphage/Sinon.JS/28

Node images default to Debian
@coveralls
Copy link

coveralls commented Jul 3, 2017

Coverage Status

Coverage remained the same at 94.998% when pulling 310c83a on fearphage:circleci-integration into 079925b on sinonjs:master.

@fatso83
Copy link
Contributor

fatso83 commented Jul 15, 2017

I didn't mean it was a config issue; I meant it was a superficial change that is functionally equivalent. As long as it works and doesn't make things harder it's all fine to me 😁

because of the problems with running tests in Node 8 on travis.

Which ones are that? I don't know of any. Superficially googled and came up with this, which seems to be a solved problem.

@fatso83
Copy link
Contributor

fatso83 commented Jul 15, 2017

Sorry, I am an idiot. Just saw the list of PRs ... #1469, of course.

@fatso83
Copy link
Contributor

fatso83 commented Jul 28, 2017

Two thoughts:

  • could it be that it picks up a phantomjs version on the local path, instead of the one we bundle? (Out on a limb with this one, but could explain the "works on my machine" issue)
  • try bumping the phantomic version to 1.5.2. We are on 1.4.0, but it only got official support for phantom 2.0 with version 1.5.2. I might not read the docs right, though ...

@fatso83
Copy link
Contributor

fatso83 commented Jul 28, 2017

This issue is what made me think of it, btw. Might not be related, but could be.

@mroderick
Copy link
Member

We should probably make it a priortiy to replace PhantomJS with headless chrome (#1405). We are likely to do this soon anyway, and it is likely to obviate many of these problems.

@fatso83
Copy link
Contributor

fatso83 commented Aug 1, 2017

While researching #1498, I understood what the failing synchronous network request was. It simply happens because ./scripts/build.js (the prepublish step) has not been called before npm test is run. Therefore there is no ../../pkg/sinon.js to load in the webworker file. The importScripts() call in WebWorkers is synchronous, as they need to load the code before anything else is run.

I'll try to get this fixed as part of my #1498 fixes.

fatso83 added a commit to fatso83/sinon that referenced this pull request Aug 1, 2017
The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious
reason with a synchronous XHR requests. The reason was most
probably the importScript('../../pkg/sinon.js') call in the
webworker file was referencing a build file that wasn't built.

By now explicitly building this file as part of the test run
this error has now disappeared locally, and probably will
disappear on CircleCI as well.
fatso83 added a commit to fatso83/sinon that referenced this pull request Aug 1, 2017
The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious
reason with a synchronous XHR requests. The reason was most
probably the importScript('../../pkg/sinon.js') call in the
webworker file was referencing a build file that wasn't built.

By now explicitly building this file as part of the test run
this error has now disappeared locally, and probably will
disappear on CircleCI as well.
@fatso83
Copy link
Contributor

fatso83 commented Aug 1, 2017

This should work as soon as #1504 lands.

@fearphage
Copy link
Member Author

This should work as soon as #1504 lands.

Thanks for looking into this!

fatso83 added a commit to fatso83/sinon that referenced this pull request Aug 1, 2017
The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious
reason with a synchronous XHR requests. The reason was most
probably the importScript('../../pkg/sinon.js') call in the
webworker file was referencing a build file that wasn't built.

By now explicitly building this file as part of the test run
this error has now disappeared locally, and probably will
disappear on CircleCI as well.

fixes the build in sinonjs#1479
fatso83 added a commit to fatso83/sinon that referenced this pull request Aug 1, 2017
The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious
reason with a synchronous XHR requests. The reason was most
probably the importScript('../../pkg/sinon.js') call in the
webworker file was referencing a build file that wasn't built.

By now explicitly building this file as part of the test run
this error has now disappeared locally, and probably will
disappear on CircleCI as well.

fixes the build in sinonjs#1479
fatso83 added a commit to fatso83/sinon that referenced this pull request Aug 1, 2017
The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious
reason with a synchronous XHR requests. The reason was most
probably the importScript('../../pkg/sinon.js') call in the
webworker file was referencing a build file that wasn't built.

By now explicitly building this file as part of the test run
this error has now disappeared locally, and probably will
disappear on CircleCI as well.

fixes the build in sinonjs#1479
fatso83 added a commit that referenced this pull request Aug 1, 2017
@fatso83
Copy link
Contributor

fatso83 commented Aug 1, 2017

No idea why you are getting EPEERINVALID

npm ERR! peerinvalid The package eslint@3.19.0 does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer eslint-config-sinon@1.0.3 wants eslint@^4.2.0
npm ERR! peerinvalid Peer eslint-plugin-mocha@4.11.0 wants eslint@^2.0.0 || ^3.0.0 || ^4.0.0

@fatso83
Copy link
Contributor

fatso83 commented Nov 8, 2017

Hi, this has been building for quite a while. Any reason not to merge it (except for the history 😛 )? For some reason, it is already being used in other PRs here, even though it hasn't been merged to master 😄

@fatso83 fatso83 merged commit e96ff8c into sinonjs:master Nov 13, 2017
@fatso83
Copy link
Contributor

fatso83 commented Nov 13, 2017

Squashed and merged. Thanks, Phred!

@fearphage fearphage deleted the circleci-integration branch July 8, 2018 16:04
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious
reason with a synchronous XHR requests. The reason was most
probably the importScript('../../pkg/sinon.js') call in the
webworker file was referencing a build file that wasn't built.

By now explicitly building this file as part of the test run
this error has now disappeared locally, and probably will
disappear on CircleCI as well.

fixes the build in sinonjs#1479
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
Uses alpine docker image (Node images default to Debian)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants