-
Notifications
You must be signed in to change notification settings - Fork 79
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
refactor: remove tests that dont apply to our repo space, and remove since we don't have control over it. Consider replacing w/ a different approach #482
Conversation
@@ -431,20 +361,7 @@ workflows: | |||
- /tagged-release\/.*/ | |||
- /run-e2e\/.*/ | |||
requires: | |||
- build |
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.
Build wasn't required here, since we already have publish_to_local_registry
which DOES depend on build. This just gets rid of some unnecessary line in the dep graph for our test runs.
@@ -6,20 +6,12 @@ export const deleteProject = async (cwd: string, profileConfig?: any, usingLates | |||
const { StackName: stackName, Region: region } = getBackendAmplifyMeta(cwd).providers.awscloudformation; | |||
await retry( | |||
() => describeCloudFormationStack(stackName, region, profileConfig), | |||
stack => stack.StackStatus.endsWith('_COMPLETE'), | |||
stack => stack.StackStatus.endsWith('_COMPLETE') || stack.StackStatus.endsWith('_FAILED'), |
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 a change I'm merging in from the CLI which John released last week. Seems like a sane check to add, but not a huge deal either way since our cleanup script should get these cases within 6 hours anyway.
} from 'amplify-e2e-core'; | ||
|
||
const defaultsSettings = { | ||
name: 'authTest', | ||
}; | ||
|
||
describe('placeholder', () => { |
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.
Removing this, which we'd disabled during migration, and getting rid of tests that don't interact w/ API.
…since we don't have control over it. Consider replacing w/ a different approach
0053689
to
053fd34
Compare
Codecov Report
@@ Coverage Diff @@
## main #482 +/- ##
=======================================
Coverage 66.24% 66.24%
=======================================
Files 228 228
Lines 14999 14999
Branches 3675 3675
=======================================
Hits 9936 9936
Misses 4624 4624
Partials 439 439 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
…since we don't have control over it. Consider replacing w/ a different approach (#482)
Description of changes
integration_test
build step since we don't have control over it, and have no mechanism to debug Consider replacing w/ a different approach, though OOS for this PR.Issue #, if available
N/A
Description of how you validated changes
yarn clean && yarn setup-dev && yarn test
, and running an e2e suite https://app.circleci.com/pipelines/github/aws-amplify/amplify-category-api/201/workflows/c0c6c51d-63a0-4f62-a776-649c525f1f93Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.