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

added a RANDOM_SEED as env-level seed for the data faker #2541

Merged
merged 19 commits into from Jan 30, 2019

Conversation

@Pomax
Copy link
Collaborator

commented Jan 29, 2019

Partial fix for #2536

@Pomax Pomax requested a review from cadecairos Jan 29, 2019

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@cadecairos
Copy link
Member

left a comment

Unless you disagree, I think we might as well add a static RANDOM_SEED value to .travis.yml before we merge.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@Pomax

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 29, 2019

Nope, I'm down with that.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@Pomax

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 29, 2019

Added seed value 530910203, which is the CRC16 for "foundation.mozilla.org" =)

@Pomax Pomax requested a review from cadecairos Jan 29, 2019

- npm run percy
after_success:
- coveralls
env:
global:
- ALLOWED_HOSTS=localhost

This comment has been minimized.

Copy link
@Pomax

Pomax Jan 29, 2019

Author Collaborator

sorted them alphabetically just because I like being able to find things =D

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 29, 2019 Inactive

@Pomax

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

Hm, it looks like I/we need to figure out how to make the RANDOM_SEED value get ignored during the standard inv test part of the run. Because a value is set at the env. level for Travis, inv test will use that seed value, and so dates in News won't follow the testing constraints that should apply when no seed is specified =S

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 30, 2019 Inactive

@Pomax

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

Resolved this by making faker instructions that need to be "compliant" during Django's manage.py test run but need to be deterministically "random" for visual regressiont esting tap into the settings.TESTING var, which tells us whether or not we're in a true test run, or whether we're "just running code".

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 30, 2019 Inactive

@Pomax

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2019

@cadecairos @patjouk I've also added the change that takes percy/cypress out of the standard install set (by making it a peerDependencies entry in package.json) and added it as an explicit install step in .travis.yml

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 30, 2019 Inactive

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 30, 2019 Inactive

important swap!
percy/cypress installs ^3.1.3, so installing 3.1.4 first means no double install for cypress occurs

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-2541 Jan 30, 2019 Inactive

@Pomax Pomax merged commit 4903cb8 into master Jan 30, 2019

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 74.073%
Details
percy/foundation.mozilla.org Visual review approved by Pomax
Details

@Pomax Pomax deleted the faker-seeding branch Jan 30, 2019

@patjouk
Copy link
Collaborator

left a comment

If you could change that comment in a new PR, that would be great!

@@ -538,10 +540,9 @@
# Buyers Guide Rate Limit Setting
BUYERS_GUIDE_VOTE_RATE_LIMIT = env('BUYERS_GUIDE_VOTE_RATE_LIMIT')

# Detect if we're testing
# Detect if we're in official testing mode

This comment has been minimized.

Copy link
@patjouk

patjouk Jan 31, 2019

Collaborator

Nitpicking after PR is merged: can you define official here? Does it mean CI or testing on dev machines?

This comment has been minimized.

Copy link
@Pomax

Pomax Jan 31, 2019

Author Collaborator

Neither - the only testing Django knows of is the kind that happens when you run manage.py test, which this lets you detect.

@Pomax Pomax referenced this pull request Jan 31, 2019
cadecairos added a commit that referenced this pull request Mar 19, 2019
added a RANDOM_SEED as env-level seed for the data faker (#2541)
* added a RANDOM_SEED as env-level seed for the data faker
* doc update to mention said var
* updated news faker to tap into settings.RANDOM_SEED
* random seed added to travis, removed --seed in script section
* bypass random seed during 'manage test'
* make percy/cypress a peerdepencency and only install during travis run
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.