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.

Sign up for GitHub

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

Jump to bottom

tests: Use a random test user when no env vars are present #584

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

tests: Use a random test user when no env vars are present #584

wants to merge 4 commits into from

Conversation

thgreasi
Copy link
Member

@thgreasi thgreasi commented Oct 1, 2018 •
edited
Loading

  • This changes the test suit to register a random test user when the respective env vars are not provided, and deletes it after the tests complete. It still uses the RESINTEST_USERNAME/... env vars if present, since while developing this I ended up having the staging api rate limit me b/c of the extra user registrations that the suit did. As a result, for practical reasons, I think we should still keep them to not add friction when developing. This enables us to
    • run the sdk tests on resinCI before secrets get supported
    • reduce the configuration needed to run the sdk tests, making easier for contributors and other team members to run them
  • Removes the RESINTEST_REGISTER_* env vars and generated a random user as well.
  • Drops travis tests, as a way to reduce the load to the API. This shouldn't reduce the effectiveness of the CI's used, since resinCI will be testing the environment that travis was testing before.
  • Still uses appveyor since
    • it's nice to test the sdk on a windows machine as well
    • it is responsible for running the paid user tests, until resinCI adds support for secrets
  • Adds the required parameters to headless chrome so that it works on resinCI

Resolves: #582
Change-type: minor
See: https://www.flowdock.com/app/rulemotion/resin-frontend/threads/JIU7143SphYOgcVeQVh5n69eycK
See: resin-io/resin-concourse#248
See: https://github.com/GoogleChrome/puppeteer/blob/master/docs/troubleshooting.md#running-on-alpine
Signed-off-by: Thodoris Greasidis [email protected]

@thgreasi thgreasi self-assigned this Oct 1, 2018
@LucianBuzzo
Copy link
Contributor

@thgreasi We should make it clear that the tests are auto-generating a user, and what the user's email is. Do we know if these test users can be blacklisted from our analytics?

@thgreasi
Copy link
Member Author

thgreasi commented Oct 1, 2018

@LucianBuzzo

We should make it clear that the tests are auto-generating a user

I will add an extra note in the tests section of README.

Do we know if these test users can be blacklisted from our analytics?

Yea, everything @resin.io isn't counted in our analytics and after discussing this w/ @pimterry & @gelbal, it seems that it's fine
See: https://www.flowdock.com/app/rulemotion/resin-frontend/threads/JIU7143SphYOgcVeQVh5n69eycK

@LucianBuzzo
Copy link
Contributor

@thgreasi The test runner should log to stdout that its using an autogenerated user, otherwise a misconfiguration could go unnoticed quite easily.

pimterry
pimterry approved these changes Oct 8, 2018
View reviewed changes
Copy link
Contributor

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this. I think we're holding it for now just because of the reliability implications, right? Once that's resolved somehow, this looks good to me.

@thgreasi thgreasi mentioned this pull request Oct 9, 2018
Merged
@thgreasi
Copy link
Member Author

@pimterry yea, that's my concern atm.
I also faced an issue configuring appveyor to use the paid account, but since the above implications still stand, I decided to postpone that for now and resolve once the reliability dust settles somehow.

@thgreasi thgreasi mentioned this pull request Nov 6, 2018
Open
@LucianBuzzo
Copy link
Contributor

@thgreasi What is the status of this PR? are we still concerned about reliability?

LucianBuzzo
LucianBuzzo reviewed Apr 29, 2019
View reviewed changes
README.md

While developing you might end up in a situation that the API returns rate limiting errors.
That's probably caused by the new user signups that each run of the test suit does.
You can avoid those errors to some extend by specifying a pre-existing **empty** user account to run the suit against.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:

  • suit -> suite
  • extend -> extent

tests/integration/setup.coffee
.tap -> shouldDeleteTestUser = true
.return(credentials)
.tapCatch ->
console.error('Failed to autogenerate test credentials!')
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should just throw an error here?

@LucianBuzzo
Copy link
Contributor

@dimitrisnl Whats the status of this PR? reviewing the comments it looks like it was nearly ready to go in November

@thgreasi
Copy link
Member Author

thgreasi commented May 7, 2019

Thanks for the review @LucianBuzzo 👍
I will catchup with this later this week and request a re-review.
WRT the reliability issues, this was discussed in an arch call and we will now be limiting balena-ci to only run the tests on a single node version.

@thgreasi
Copy link
Member Author

thgreasi commented May 7, 2019

@balena-ci retest

@thgreasi thgreasi removed the blocked label May 10, 2019
@thgreasi
Copy link
Member Author

thgreasi commented Jun 3, 2019

It seems that this is blocked again, since we have decreased the allowed rate of signups.

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

@LucianBuzzo LucianBuzzo LucianBuzzo left review comments

@pimterry pimterry pimterry approved these changes

@dimitrisnl dimitrisnl Awaiting requested review from dimitrisnl

Assignees

@thgreasi thgreasi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should be able to run the tests on balenaCI
3 participants
@thgreasi @LucianBuzzo @pimterry