Skip to content

ci: enable production integration testing #6886

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
Merged
wlee221 merged 7 commits into from
Oct 1, 2020
Merged

ci: enable production integration testing #6886

wlee221 merged 7 commits into from
Oct 1, 2020

Conversation

@wlee221
Copy link
Contributor

@wlee221 wlee221 commented Sep 29, 2020 •
edited
Loading

Issue #, if available:

Description of changes: This PR updates integration tests to run on both dev builds and prod builds. This also resolves sideEffects regression in aws-amplify-react that was found along the way.

I considered creating a separate job for prod testing, but this was inefficient because we would have to run persist_to_workspace for each of our sample to transition from dev to prod.

I've tested that all our browser integration tests pass on both prod and dev with the accompanying PR on samples repo. See link to see what this looks like on circleCI!

Screen Shot 2020-09-29 at 10 28 47 AM

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Sep 29, 2020 •
edited
Loading

Codecov Report

Merging #6886 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6886   +/-   ##
=======================================
  Coverage   73.29%   73.29%           
=======================================
  Files         213      213           
  Lines       13191    13191           
  Branches     2486     2486           
=======================================
  Hits         9668     9668           
  Misses       3359     3359           
  Partials      164      164

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16aaa05...41083f4. Read the comment docs.

@wlee221 wlee221 marked this pull request as ready for review September 29, 2020 17:26
@wlee221 wlee221 requested a review from iartemiev as a code owner September 29, 2020 17:26
wlee221
wlee221 commented Sep 29, 2020
View reviewed changes
packages/aws-amplify-react/package.json
@@ -1,108 +1,120 @@
{
"name": "aws-amplify-react",
Copy link
Contributor Author

@wlee221 wlee221 Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Changed spaces to tabs as per our .prettierrc config while I was here. Turn on "Hide whitespace changes" :D

ashika01
ashika01 reviewed Sep 29, 2020
View reviewed changes
@wlee221
Copy link
Contributor Author

wlee221 commented Sep 29, 2020

@ashika01

File sizes after gzip:

  117.26 KB (-58.61 KB)  build/static/js/2.ac17f930.chunk.js
  65.41 KB (+64.36 KB)   build/static/js/main.cfbc14b9.chunk.js
  3.62 KB                build/static/css/2.e5012852.chunk.css
  762 B                  build/static/js/runtime~main.a8a9905a.js
  514 B                  build/static/css/main.38b8828e.chunk.css

iartemiev
iartemiev approved these changes Sep 29, 2020
View reviewed changes
Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

Looks good William! 👍🏻 Thanks for including the CCI pipeline run

ashika01
ashika01 approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@ashika01 ashika01 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Lets get this merged. Lets keep an eye out for bundle size. Its ok since aws-amplify-react is in maintenance mode rn

sammartinez
sammartinez approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM and im OK with the bundle size increase. Thanks for this @wlee221 !

ericclemmons
ericclemmons reviewed Sep 30, 2020
View reviewed changes
packages/aws-amplify-react/package.json
Comment on lines +9 to +10
"./src/Storage/index.ts",
"./src/Storage/S3Album.ts",
Copy link
Contributor

@ericclemmons ericclemmons Sep 30, 2020

Choose a reason for hiding this comment

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

How are these considered side-effects?

https://github.com/aws-amplify/amplify-js/blob/d2f4bca73efda7375cf5f33bcffef88938f6c803/packages/aws-amplify-react/src/Storage/index.tsx

Copy link
Contributor Author

@wlee221 wlee221 Sep 30, 2020

Choose a reason for hiding this comment

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

To be honest, I'm not sure :/ But I confirmed manually that all three ./src/index.ts, ./src/Storage/index.ts, and ./src/Storage/S3Album.ts need to be marked sideEffects or modules fail to load. My guess is that the app is expecting S3Album, and all files leading up to it (index.ts -> Storage/index.ts -> src/Storage/S3Album.ts) cannot not be swept.

@ericclemmons
Copy link
Contributor

ericclemmons commented Sep 30, 2020

Is there a benefit to running tests in both dev & prod mode? In terms of a test, until we have dev-specific features, it seems like doubling up on CI resources when prod is what matters.

@wlee221
Copy link
Contributor Author

wlee221 commented Oct 1, 2020

With our discussion today, I'll go ahead and merge this. Thanks everyone 👍

@wlee221 wlee221 merged commit 401da25 into main Oct 1, 2020
nubpro pushed a commit to nubpro/amplify-js that referenced this pull request Oct 2, 2020
@wlee221 @ashika01 @nubpro
ci: enable production integration testing (aws-amplify#6886)
aad7b49 
* Add production testing to ci

* Update side effects

Co-authored-by: Ashika <[email protected]>
iartemiev pushed a commit that referenced this pull request Oct 13, 2020
@wlee221 @ashika01 @iartemiev
ci: enable production integration testing (#6886)
cbc88f7 
* Add production testing to ci

* Update side effects

Co-authored-by: Ashika <[email protected]>
@wlee221 wlee221 deleted the integ-prod-test/main branch November 18, 2020 19:50
@github-actions
Copy link

github-actions bot commented Nov 19, 2021

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@iartemiev iartemiev iartemiev approved these changes

+3 more reviewers

@ericclemmons ericclemmons ericclemmons left review comments

@sammartinez sammartinez sammartinez approved these changes

@ashika01 ashika01 ashika01 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wlee221 @ericclemmons @sammartinez @iartemiev @ashika01