-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
This approach seems good. One thing I've just discovered with my bootstrap test, is testing the module instead of doing it from the command line ( |
3b4acf4
to
b5afab9
Compare
Mmm, good point. I wonder if there's something we can do to test the CLI interface specifically. |
This should now be ready to review, so I've removed the "WIP" prefix and squashed the changes. |
@@ -26,6 +26,10 @@ var readYaml = require('read-yaml'); | |||
var travisEncrypt = promisify(require('travis-encrypt')); | |||
var writeYaml = require('write-yaml'); | |||
|
|||
// Uncomment this to record network requests/responses for automated tests. | |||
// var nock = require('nock'); | |||
// nock.recorder.rec(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever implement live tests with real GitHub repositories, we should enable this by default (when the test is running) to have more info when such a test fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever implement live tests with real GitHub repositories, we should enable this by default (when the test is running) to have more info when such a test fails.
Indeed. On the other hand, we can't enable this when running automated tests against the mocks, since enabling it disables those mocks. I've added some commentary to explain these use cases in c28db8a.
Looks good to me, I have a single doubt. Some tests are testing corner cases and test configure up to a certain step. In theory the code that handles the corner cases shouldn't have side effects, but I think it'd be better to always test up to the end (.travis.yml check included) to make sure that's actually the case. You could add a function that checks everything and call it instead of cancel. |
Needs conflict resolution. |
Indeed, it's a good point.
Hmm, yes. I've been looking at the code, and it isn't completely obvious how best to do this, but I'll tackle it next. |
The test failure is apparently because the tests all stall and time out on |
I've done this in 00314a9, so now all tests (except for one) allow the configure command to run to completion and check that it completed successfully and wrote a correct .travis.yml file. This also enabled me to consolidate some tests that became duplicates once they all ran to completion. The exception is the first test, which checks that the command outputs a welcome message with the repository slug. Because the command immediately prompts the user to enter their credentials after displaying that message, it races the await call that would see that prompt and respond to it. And I'm not sure quite how to restructure the code to eliminate that race condition. So I left that one alone. In 5dbed58, I also added a test for one more case that wasn't being tested: the repository is listed in Travis, but it isn't active, so the command activates it. Finally, in d5c4096, I worked around the test failures by running mocha directly in Travis via |
Right after I wrote that, I figured it out, so b97f0b9 restructures the code to win the race and let that test run to completion. |
Looks like |
test the configure command (fixes #35)
Filed #100 to investigate the |
@marco-c , @brendandahl This is a WIP of a test for the configure command. It uses nock to mock an HTTP request/response, so we can test the parts of the command that call the GitHub and Travis APIs without actually needing network access.
So far the test just checks that the introductory message is printed, the user is prompted for a username and password, and the user is prompted to re-enter the username/password if it's incorrect. (That last check is the only one that makes a network request so far, but this is a proof-of-concept that we could use mock those requests).