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

feat(pr-baseline-2): Modify ADO PR comments to include baseline info #909

Merged
merged 22 commits into from Oct 26, 2021

Conversation

lisli1
Copy link
Contributor

@lisli1 lisli1 commented Oct 23, 2021

Details

Added the different scenarios for the PR comments in ADO:

  1. Baseline not configured (user does not specify a baselineFile as input to trigger baselining)
  2. Baseline not detected (first time running so baseline file not present)
  3. New failures + existing failures in baseline
  4. No new failures + existing failures in baseline
  5. New failures + no existing failures in baseline (assume same as (2), where baseline would not be detected because there's no need to add a baseline file if there were no errors)
  6. No new failures + no existing failures in baseline

Note: The snapshot for result-markdown-builder is long but it's worth reviewing for the different PR comment scenarios

Motivation

Addresses part of WI 1882596 for feature work

Context
  • Will update the artifacts/scan arguments URL and commit hash in the PR comment title in a future PR to keep this one shorter.
  • Will also update baselining docs URL in a future PR when the docs are written/available.
  • This PR doesn't include piping the baselineEvaluation from the CombinedScanResult in the scanner to the ADOPullRequestCommentCreator.
  • baseline-types.ts is a temp file until ai-scan package gets updated and we can consume the baseline-types from that.

Pull request checklist

  • Partially addresses an existing issue: WI 1882596
  • Added relevant unit test for your changes. (yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • Ran precheckin (yarn precheckin)
  • (after PR created) The Accessibility Checks (pull_request) check should fail. All other checks should pass.

@lisli1 lisli1 requested a review from a team as a code owner October 23, 2021 02:35
@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2021

Accessibility Insights Accessibility Insights Action

  • URLs: 1 URL(s) failed, 3 URL(s) passed, and 0 were not scannable
  • Rules: 6 check(s) failed, 11 check(s) passed, and 37 were not applicable
  • Download the Accessibility Insights artifact to view the detailed results of these checks

Failed instances

  • 3 × label: Ensures every form element has a label
  • 1 × html-has-lang: Ensures every HTML document has a lang attribute
  • 1 × image-alt: Ensures <img> elements have alternate text or a role of none or presentation
  • 1 × input-image-alt: Ensures <input type="image"> elements have alternate text

This scan used axe-core 4.3.2 with Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.75 Safari/537.36.

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2021

Codecov Report

Merging #909 (ab79e19) into main (134dc8d) will increase coverage by 0.93%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #909      +/-   ##
==========================================
+ Coverage   87.61%   88.55%   +0.93%     
==========================================
  Files          39       39              
  Lines         840      909      +69     
  Branches      102      113      +11     
==========================================
+ Hits          736      805      +69     
  Misses        104      104              
Impacted Files Coverage Δ
...r/pull-request/ado-pull-request-comment-creator.ts 99.00% <100.00%> (+0.05%) ⬆️
.../shared/src/mark-down/report-markdown-convertor.ts 100.00% <100.00%> (ø)
...es/shared/src/mark-down/result-markdown-builder.ts 100.00% <100.00%> (ø)

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 134dc8d...ab79e19. Read the comment docs.

let reportMarkdown: string;
const baselineFileName = this.adoTaskConfig.getBaselineFile();

if (baselineFileName === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to push this into a function whose name provides some clarity about what it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean specifically the part for checking if baselining was configured by the user (just L99-101)?

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I was thinking 98-117 would be the new method. Looking again, you could wrap lines 108-110 in a simple method to get the BaselineInfo, then call it something like this:

        const reportMarkdown = this.reportMarkdownConvertor.convert(
            combinedReportResult,
            AdoPullRequestCommentCreator.CURRENT_COMMENT_TITLE,
            this.getBaselineInfo(this.adoTaskConfig.getBaselineFile(), baselineEvaluation)
        );

...

    function getBaselineInfo(baselineFileName: string, baselineEvaluation: BaselineEvaluation) {
        if (!baselineFileName) {
            return {} as BaselineInfo;
        }

        return { baselineFileName, baselineEvaluation };
    }

I wouldn't consider this as a "must have", but it feels like it would improve the readability a bit. I remember hearing someone on the team comment that a let keyword is a sort of a code smell that can usually be improved with a simple refactor.

lisli1 and others added 3 commits October 25, 2021 11:56
Co-authored-by: Dave Tryon <45672944+DaveTryon@users.noreply.github.com>
…essibility-insights-action into lisli1/pr-comment-baseline
Copy link
Contributor

@DaveTryon DaveTryon left a comment

Choose a reason for hiding this comment

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

Wording seems a bit odd, but approving either way

lisli1 and others added 2 commits October 25, 2021 17:00
Co-authored-by: Dave Tryon <45672944+DaveTryon@users.noreply.github.com>
@lisli1 lisli1 merged commit b1d60c7 into main Oct 26, 2021
@lisli1 lisli1 deleted the lisli1/pr-comment-baseline branch October 28, 2021 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants