Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Simpler Cypress Setup #150
Simpler Cypress Setup #150
Changes from all commits
18eb410
9909161
2d4687e
aa2b47c
4143739
d4e0890
b12c0e4
a420569
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
caching doesn't work for
deployment_status
events sadly. (known bug)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.
Is this a big deal at the moment ? No right our apps arent that big that it would mean a huge slow down?
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.
It's not really a big deal but it will add time to the test running. So if you push one final commit to the bootstrapper you'll need to wait an extra minute or so.
This does NOT impact the generated projects so definitely not a huge deal in my mind.
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.
ahhh okay good call out!
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.
Removed the
jwalton/gh-find-current-pr@v1
action dependency as we no longer need to "figure out" what the URL is for the deployment. Heroku gives that to as when it creates the status event.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.
This is why the
Setup
step above is now a LOT simplerThere 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.
This Github Action used to do 2 things:
1 - Tell Heroku if it needs to build a Vue app or a React app
2 - Trigger Cypress and cause it to fail early so...(blah blah complicated logic that doesn't exist anymore)
Now all this action does is detect if the dev has touched more React files or more Vue files, and then tell Heroku to change it's build type if a change is needed.
I still don't love that this runs on EVERY commit. I may try to do something smarter, but thats a future PR.
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.
All the comments I made above for the bootstrappers
cypress.yml
are relevant here as wellThere 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.
buuut....since this file is even simpler now, we don't even need a
Setup
step anymoreThis file was deleted.
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.
What about test data? should that not get triggered?
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.
Test data gets run once during review app creation. That happens in a different place than this
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.
tn-spa-bootstrapper/{{cookiecutter.project_slug}}/scripts/db_setup.sh
Line 6 in d2be249
db_setup
is what triggers test data to be created.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.
which is called from the
postdeploy
Heroku step.tn-spa-bootstrapper/{{cookiecutter.project_slug}}/app.json
Line 47 in 4197058
That step only ever gets triggered during review app creation and thus never runs on staging or prod.
Which is why for brand new projects you have to run some of these commands manually once against staging.
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.
Heroku requires that our URLs use
-
instead of_
. Our slugs are the opposite (to be Pythonic).Annoying and an issue you run into if you bootstrap a project from scratch that has a space in it's name.
The fix isn't always obvious. But I think this is now a solved problem.
This file was deleted.
This file was deleted.
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.
same as above. Heroku requires
-
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.
Shouldnt be an issue when we remove heroku default in the BS
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.
Correct, hopefully we can find a way to have separate heroku scripts set values like these for the project based on what was actually created in heroku. Obviously this code is effectively guessing what the URL will be (in a deterministic, but fragile way)