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 got, fix tests, and update .travis.yml #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sonicdoe
Copy link

@sonicdoe sonicdoe commented Mar 7, 2017

No description provided.

Copy link
Owner

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

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

Can you also change .travis.yml to test against 6 and 4 instead of 5 and 4?

package.json Outdated
@@ -36,9 +36,9 @@
"got": "^6.3.0"
},
"devDependencies": {
"ava": "*",
Copy link
Owner

Choose a reason for hiding this comment

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

This is on purpose, same for XO. I do this together with @sindresorhus and some others to make sure that we always test our own projects against the latest versions. This way we can detect bugs from those libraries early on.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, thanks for the clarification.

@sonicdoe sonicdoe changed the title Fix linting and pin XO and AVA Fix linting and update .travis.yml Mar 7, 2017
@SamVerschueren
Copy link
Owner

Have to look into the Travis tests. Not sure why it fails, maybe because it runs on Travis and they are throttled. Does it work locally for you?

@sonicdoe
Copy link
Author

sonicdoe commented Mar 7, 2017

At first I thought it was caused by rate limiting, too. However, the tests didn’t work for me locally as well. I have tried to fix the tests in #2.

@sonicdoe sonicdoe changed the title Fix linting and update .travis.yml Update got, fix tests, and update .travis.yml May 29, 2017
@sonicdoe
Copy link
Author

Since got v7 was released today, I have updated this pull request. The tests are now working locally for me (as long as I have a valid TRAVIS_TOKEN set). @SamVerschueren Could you take a look again?

@SamVerschueren
Copy link
Owner

Do you have any idea why the tests are failing? Hard to tell on mobile :)

@SamVerschueren
Copy link
Owner

Sorry, didn't read your last comment. Will have a look tomorrow.

Assigning a property on `process.env` will implicitly convert the value to a string, see https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_process_env. This meant that if TRAVIS_TOKEN was unset it was re-set to the string `undefined`.
@sonicdoe
Copy link
Author

Well, at least you prompted me to look into this in more detail :)

The actual issue was that if TRAVIS_TOKEN was not set when running the tests, the “global token option” test would re-set it using process.env.TRAVIS_TOKEN = token. However, assigning a property on process.env will implicitly convert the value to a string which meant that process.env.TRAVIS_TOKEN was literally assigned the string undefined.

I have now fixed this by simply setting process.env.TRAVIS_TOKEN to an empty string in case of TRAVIS_TOKEN being undefined.

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.

2 participants