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

Various small sdk test improvements #437

Merged
merged 4 commits into from Nov 28, 2017

Conversation

Projects
None yet
2 participants
@pimterry
Member

pimterry commented Nov 23, 2017

This PR:

  • Prints browser console logs during tests, so you can tell what's going on
  • Removes the USER_ID from the test config

The user id is that main change: with that gone, we can use the same config on multiple environments (if the login is the same). This is going to let us start running some of the tests against the multicontainer environment shortly.

@pimterry pimterry requested a review from thgreasi Nov 23, 2017

@resin-io-versionbot

This comment has been minimized.

Show comment
Hide comment
@resin-io-versionbot

resin-io-versionbot bot Nov 23, 2017

Contributor

@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit.

Contributor

resin-io-versionbot bot commented Nov 23, 2017

@pimterry, status checks have failed for this PR. Please make appropriate changes and recommit.

@pimterry

This comment has been minimized.

Show comment
Hide comment
@pimterry

pimterry Nov 23, 2017

Member

Looks like the failing test is just the one that #435 will fix, should be good once that's merged and this is rebased.

Member

pimterry commented Nov 23, 2017

Looks like the failing test is just the one that #435 will fix, should be good once that's merged and this is rebased.

it 'should eventually be a user id', ->
resin.auth.getUserId()
.then (userId) ->
m.chai.expect(userId).to.be.a('number')

This comment has been minimized.

@thgreasi

thgreasi Nov 24, 2017

Member

So, it's a funny realization that this is the only place that credentials.userId was used.
I'm fine with this as it is, but since this is the so famous userId, would it make any sense to also cross-check it with a server returned value?
eg: requesting the user with that exact username from the api

Or at least, how about verifying that it's a positive integer?

@thgreasi

thgreasi Nov 24, 2017

Member

So, it's a funny realization that this is the only place that credentials.userId was used.
I'm fine with this as it is, but since this is the so famous userId, would it make any sense to also cross-check it with a server returned value?
eg: requesting the user with that exact username from the api

Or at least, how about verifying that it's a positive integer?

This comment has been minimized.

@pimterry

pimterry Nov 28, 2017

Member

I think doing a separate pine request to confirm feels a bit like overkill imo. I've added a bonus check to make sure it's a positive integer though.

@pimterry

pimterry Nov 28, 2017

Member

I think doing a separate pine request to confirm feels a bit like overkill imo. I've added a bonus check to make sure it's a positive integer though.

This comment has been minimized.

@thgreasi

thgreasi Nov 28, 2017

Member

Woohoo!

@thgreasi

thgreasi Nov 28, 2017

Member

Woohoo!

pimterry added some commits Nov 15, 2017

Stop requiring tests to be configured with the test user id
This makes it easier to share test config between environments: you need
a username & password that exists on both, but you don't need literally
the same database, which is nice.

Change-Type: patch
@resin-io-versionbot

This comment has been minimized.

Show comment
Hide comment
@resin-io-versionbot

resin-io-versionbot bot Nov 28, 2017

Contributor

VersionBot failed to carry out a status check for the above pull request here: #437. The reason for this is:
2 of 5 required status checks have not succeeded: 1 expected and 1 pending.
Please carry out relevant changes or alert an appropriate admin.

Contributor

resin-io-versionbot bot commented Nov 28, 2017

VersionBot failed to carry out a status check for the above pull request here: #437. The reason for this is:
2 of 5 required status checks have not succeeded: 1 expected and 1 pending.
Please carry out relevant changes or alert an appropriate admin.

@pimterry pimterry referenced this pull request Nov 28, 2017

Merged

Start using Typescript #438

@resin-io-versionbot resin-io-versionbot bot merged commit b7a8afb into master Nov 28, 2017

6 checks passed

AutoMerges PR merging is in progress
Reviewers 1/1 review approvals met
Versionist Found all required commit footer tags
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@resin-io-versionbot resin-io-versionbot bot deleted the sdk-test-improvements branch Nov 28, 2017

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