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

🏗 ✨ ❄️ add karma-junit-reporter to store test results in a way circleci can interpret them #33682

Merged
merged 8 commits into from
Apr 12, 2021
Merged

🏗 ✨ ❄️ add karma-junit-reporter to store test results in a way circleci can interpret them #33682

merged 8 commits into from
Apr 12, 2021

Conversation

rileyajones
Copy link
Contributor

CircleCI can show test metrics but is not currently configured to do so. Unfortunately our current approach result collection is not supported so I am adding in the reporter they recommend (https://circleci.com/docs/2.0/collect-test-data/#karma) and configuring the build to read those results

@rileyajones rileyajones force-pushed the test-data branch 5 times, most recently from 06d1a3f to 557aaf9 Compare April 7, 2021 21:22
@rileyajones rileyajones marked this pull request as ready for review April 7, 2021 22:09
@amp-owners-bot
Copy link

amp-owners-bot bot commented Apr 7, 2021 •
edited
Loading

Hey @rsimha! These files were changed:

.circleci/config.yml

rsimha
rsimha reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Exciting to see this in action! A few comments below.

rsimha
rsimha reviewed Apr 8, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Getting closer! A few more comments below.

@rileyajones rileyajones force-pushed the test-data branch 2 times, most recently from 588893a to 3a5b506 Compare April 9, 2021 18:19
rsimha
rsimha reviewed Apr 9, 2021
View reviewed changes
rsimha
rsimha reviewed Apr 9, 2021
View reviewed changes
build-system/tasks/e2e/mocha-report-multiplexer.js Outdated
const {Base} = Mocha.reporters;

/**
* Creates a custom Mocha reporter which runs many reporters at once.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we now have two ways to combine reporters: This function, and the one below, which combines MochaDotsReporter and JsonReporter.

amphtml/build-system/tasks/e2e/mocha-ci-reporter.js

Lines 27 to 32 in e9cb065

function ciReporter(runner, options) {
Base.call(this, runner, options);
this._mochaDotsReporter = new MochaDotsReporter(runner, options);
this._jsonReporter = new JsonReporter(runner, options);
return this;
}

Assuming I'm right, can we consolidate them into one solution (whichever is better)? Okay to punt this to a separate PR, and might be worth getting this bit reviewed by Esther.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at ciReporter when I was writing this and didn't view it as very extensible. I think it could easily be replaced with an invocation of my new method but as you suggest I would rather handle that as a separate PR. @estherkim Do you have an opinion on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather have ciReporter replaced directly by your junit reporter than in this roundabout way. Ok to do in a separate PR if you want to remove e2e changes out of this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll move the e2e changes to another PR

@rileyajones rileyajones force-pushed the test-data branch 3 times, most recently from 2f01eca to d9aad91 Compare April 9, 2021 22:32
rsimha
rsimha approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha 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.

Thank you for patiently addressing all comments. LGTM and...

image

😃

estherkim
estherkim requested changes Apr 12, 2021
View reviewed changes
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

Blocking for now to get e2e comments resolved, thanks!

@rileyajones rileyajones merged commit 69c41e1 into ampproject:main Apr 12, 2021
@rileyajones rileyajones mentioned this pull request Apr 12, 2021
Merged
rileyajones added a commit to rileyajones/amphtml that referenced this pull request Apr 16, 2021
@rileyajones
🏗 ✨ ❄️ add karma-junit-reporter to store test results in a way circle…
b563b84 
…ci can interpret them (ampproject#33682)

* add karma-junit-reporter to store test results in a way circleci can interpret them
rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
@rileyajones @rochapablo
🏗 ✨ ❄️ add karma-junit-reporter to store test results in a way circle…
1275772 
…ci can interpret them (ampproject#33682)

* add karma-junit-reporter to store test results in a way circleci can interpret them
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

@estherkim estherkim estherkim approved these changes

@danielrozenberg danielrozenberg Awaiting requested review from danielrozenberg

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rileyajones @rsimha @estherkim @ampprojectbot