Skip to content

Commit

Permalink
Chore: Make validation pipelines more sensitive and meaningful (#1874)
Browse files Browse the repository at this point in the history
#### Details

Our validation pipeline is not very good at surfacing customer-impacting
failures. It intentionally contains "should succeed" and "should fail"
test cases, but failures from the "should fail" cases could mean "we
completed the analysis and found accessibility errors" or "we failed to
complete the analysis". ADO returns both cases as "partially
successful", and the differences are only apparent by drilling down into
the logs. This is why the puppeteer failure went unnoticed for so long.

This PR improves validation usefulness by splitting the validation into
2 jobs--one that runs the tests, and one that validates the artifacts
generated while running those tests. If any of the test cases fail to
run, it will cause the set of artifacts to change, and the validation
job will fail. If the validation job fails, then ADO will report the
overall build status as "failed" instead of "partially succeeded".

The validation job intentionally checks only the _presence_ of the
expected artifact files, without validating the _contents_ of the
expected artifact files. This allows us to continue to use the same
template across Canary, Insider, and Production, even if the artifact
content varies due to changes in our code or in a package that we
consume (examples: ai-scan, axe-core). Artifact content validation is
already handled in the PR and CI builds, where the code and the tests
can be kept tightly synchronized.

This PR also reclassifies the azure portal test from "should pass" (a
classification added when we weren't 100% sure of the actual status due
to the puppeteer problem) to "should fail" because it scans a page with
a known accessibility error. It also puts the "should fail" tests that
generate no artifacts at the end of the list of "should fail" tests that
_do_ generate artifacts. This results in more intuitive naming for the
"should fail" artifacts.

Validation build using this change is at
https://dev.azure.com/accessibility-insights-private/Accessibility%20Insights%20(private)/_build/results?buildId=48232&view=results.

Review note: This PR involves adding some additional indentation to the
old job. It's a lot easier if you select the "Hide whitespace" option
when reviewing. It might also be useful to review it on a per-commit
basis. The reclassification went in first, followed by adding the
validation job.

##### Motivation

Make it easier to detect customer-impacting failures via the validation
pipeline

##### Context

<!-- Are there any parts that you've intentionally left out-of-scope for
a later PR to handle? -->

<!-- Were there any alternative approaches you considered? What
tradeoffs did you consider? -->

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [n/a] Addresses an existing issue: Fixes
- [n/a] Added relevant unit test for your changes. (`yarn test`)
- [n/a] Verified code coverage for the changes made. Check coverage
report at: `<rootDir>/test-results/unit/coverage`
- [x] Ran precheckin (`yarn precheckin`)
  • Loading branch information
DaveTryon authored Oct 24, 2023
1 parent 3ec4a71 commit 0db1890
Showing 1 changed file with 181 additions and 123 deletions.
304 changes: 181 additions & 123 deletions pipelines/ado-extension-validation-template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,126 +15,184 @@ parameters:
variables:
- group: 'ado-auth-example-secrets'

steps:
- task: NodeTool@0
inputs:
versionSpec: '16.x'
displayName: Use Node 16.x
# reused by all "url" cases
- script: npx serve "$(System.DefaultWorkingDirectory)/dev/website-root" -l 5858 &
displayName: 'Start /dev/website-root test server at http://localhost:5858'

# Please keep all "should succeed" cases together as a block before and "should fail" cases.
# Do not specify a condition so that the pipeline will fail if any of these fail.
- task: ${{ parameters.taskUnderTest }}
displayName: '[should succeed] case succeed-1: staticSiteDir, no baseline, failOnAccessibilityError:false'
inputs:
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
failOnAccessibilityError: false
outputArtifactName: accessibility-reports-case-succeed-1
scanTimeout: 180000

- task: ${{ parameters.taskUnderTest }}
displayName: '[should succeed] case succeed-2: up-to-date baseline'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-succeed-2'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/up-to-date-5858.baseline'

# The continueOnError for this case reflects a known issue with the task that will be fixed in a future release
- task: ${{ parameters.taskUnderTest }}
displayName: '[should succeed] case succeed-3: url, azure portal auth test'
inputs:
url: 'https://portal.azure.com'
serviceAccountName: $(a11yinsscan-service-acct-username)
serviceAccountPassword: $(a11yinsscan-service-acct-password)
authType: 'AAD'
outputArtifactName: 'accessibility-reports-case-succeed-3'
continueOnError: true

# Please keep all "should fail" cases together as a block after all "should succeed" cases
# Each case will need to have continueOnError: true. Each case but the first will also need
# a condition: succeededOrFailed() so that the pipeline will run correctly.
# Note that the first case _temporarily_ has a condition: succeededOrFailed(). This will
# be removed once we fix the issue that is causing the succeed-3 case to fail.
- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-1: staticSiteDir, no baseline'
inputs:
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
# intentionally omits artifactName; should go to default accessibility-reports
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-2: url, no baseline'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-fail-2'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-3: baseline missing failures'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-fail-3'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/missing-failures-5858.baseline'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-4 baseline with resolved failures'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-fail-4'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/with-resolved-failures-5858.baseline'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-5: new baseline file'
inputs:
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
outputArtifactName: 'accessibility-reports-case-fail-5'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/doesnt-exist.baseline'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-6: url, custom artifact upload step'
inputs:
url: 'http://localhost:5858'
uploadOutputArtifact: false
outputDir: '_accessibility-reports-case-fail-6'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-7: invalid inputs (no site specified)'
inputs:
outputArtifactName: 'accessibility-reports-case-fail-7'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-8: invalid inputs (both url and staticSiteDir specified)'
inputs:
url: 'http://localhost:5858'
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
outputArtifactName: 'accessibility-reports-case-fail-8'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-9: v1 inputs specified'
inputs:
repoServiceConnectionName: 'this isnt used anymore'
# siteDir instead of staticSiteDir
siteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
condition: succeededOrFailed()
continueOnError: true

- publish: '$(System.DefaultWorkingDirectory)/_accessibility-reports-case-fail-6'
displayName: 'custom report artifact upload for case fail-6'
artifact: 'accessibility-reports-case-fail-6'
condition: succeededOrFailed()
jobs:
- job: run_tests
displayName: 'Run tests'
pool:
name: a11y-ado-auth
steps:
- task: NodeTool@0
inputs:
versionSpec: '16.x'
displayName: Use Node 16.x
# reused by all "url" cases
- script: npx serve "$(System.DefaultWorkingDirectory)/dev/website-root" -l 5858 &
displayName: 'Start /dev/website-root test server at http://localhost:5858'

# Please keep all "should succeed" cases together as a block before and "should fail" cases.
# Do not specify a condition so that the pipeline will fail if any of these fail.
- task: ${{ parameters.taskUnderTest }}
displayName: '[should succeed] case succeed-1: staticSiteDir, no baseline, failOnAccessibilityError:false'
inputs:
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
failOnAccessibilityError: false
outputArtifactName: accessibility-reports-case-succeed-1
scanTimeout: 180000

- task: ${{ parameters.taskUnderTest }}
displayName: '[should succeed] case succeed-2: up-to-date baseline'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-succeed-2'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/up-to-date-5858.baseline'

# Please keep all "should fail" cases together as a block after all "should succeed" cases
# Each case will need to have continueOnError: true. Each case but the first will also need
# a condition: succeededOrFailed() so that the pipeline will run correctly.
- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-1: staticSiteDir, no baseline'
inputs:
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
# intentionally omits artifactName; should go to default accessibility-reports
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-2: url, no baseline'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-fail-2'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-3: baseline missing failures'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-fail-3'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/missing-failures-5858.baseline'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-4 baseline with resolved failures'
inputs:
url: 'http://localhost:5858'
outputArtifactName: 'accessibility-reports-case-fail-4'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/with-resolved-failures-5858.baseline'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-5: new baseline file'
inputs:
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
outputArtifactName: 'accessibility-reports-case-fail-5'
baselineFile: '$(System.DefaultWorkingDirectory)/dev/website-baselines/doesnt-exist.baseline'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-6: url, custom artifact upload step'
inputs:
url: 'http://localhost:5858'
uploadOutputArtifact: false
outputDir: '_accessibility-reports-case-fail-6'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-7: url, azure portal auth test'
inputs:
url: 'https://portal.azure.com'
serviceAccountName: $(a11yinsscan-service-acct-username)
serviceAccountPassword: $(a11yinsscan-service-acct-password)
authType: 'AAD'
outputArtifactName: 'accessibility-reports-case-fail-7'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-8: invalid inputs (no site specified)'
inputs:
outputArtifactName: 'no-artifact-will-be-created-here'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-9: invalid inputs (both url and staticSiteDir specified)'
inputs:
url: 'http://localhost:5858'
staticSiteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
condition: succeededOrFailed()
continueOnError: true

- task: ${{ parameters.taskUnderTest }}
displayName: '[should fail] case fail-10: v1 inputs specified'
inputs:
repoServiceConnectionName: 'this isnt used anymore'
# siteDir instead of staticSiteDir
siteDir: '$(System.DefaultWorkingDirectory)/dev/website-root'
condition: succeededOrFailed()
continueOnError: true

- publish: '$(System.DefaultWorkingDirectory)/_accessibility-reports-case-fail-6'
displayName: 'custom report artifact upload for case fail-6'
artifact: 'accessibility-reports-case-fail-6'
condition: succeededOrFailed()

- job: validate_artifacts
dependsOn: run_tests
displayName: 'Validate artifacts'
pool:
name: a11y-ado-auth
steps:
- task: DownloadPipelineArtifact@2
displayName: 'Download artifacts from running tests'
inputs:
allowPartiallySucceededBuilds: true
targetPath: '$(Pipeline.Workspace)/artifacts'

- task: CmdLine@2
displayName: 'Validate index.html files from artifacts'
# Note: Update the list of expected files as needed for changing test cases
inputs:
script: |
echo artifacts/accessibility-reports-case-fail-2/index.html > expected-index-html-files.txt
echo artifacts/accessibility-reports-case-fail-3/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports-case-fail-4/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports-case-fail-5/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports-case-fail-6/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports-case-fail-7/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports-case-succeed-1/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports-case-succeed-2/index.html >> expected-index-html-files.txt
echo artifacts/accessibility-reports/index.html >> expected-index-html-files.txt
echo Expected index.html files \(expected-index-html-files.txt\):
cat expected-index-html-files.txt
echo
echo Actual index.html files from artifacts \(actual-index-html-files.txt\):
find artifacts -name index.html | sort | tee actual-index-html-files.txt
echo
echo Comparing actual-index-html-files.txt to expected-index-html-files.txt
# Note: diff will return 0 if the files are identical, 1 if they differ, and 2 if an error occurs
diff actual-index-html-files.txt expected-index-html-files.txt
workingDirectory: '$(Pipeline.Workspace)'

- task: CmdLine@2
displayName: 'Validate .baseline files from artifacts'
# Note: Update the list of expected files as needed for changing test cases
inputs:
script: |
echo artifacts/accessibility-reports-case-fail-3/missing-failures-5858.baseline > expected-baseline-files.txt
echo artifacts/accessibility-reports-case-fail-4/with-resolved-failures-5858.baseline >> expected-baseline-files.txt
echo artifacts/accessibility-reports-case-fail-5/doesnt-exist.baseline >> expected-baseline-files.txt
echo Expected .baseline files \(expected-baseline-files.txt\):
cat expected-baseline-files.txt
echo
echo Actual .baseline files from artifacts \(actual-baseline-files.txt\):
find artifacts -name *.baseline | sort | tee actual-baseline-files.txt
echo
echo Comparing actual-baseline-files.txt to expected-baseline-files.txt
# Note: diff will return 0 if the files are identical, 1 if they differ, and 2 if an error occurs
diff actual-baseline-files.txt expected-baseline-files.txt
workingDirectory: '$(Pipeline.Workspace)'

0 comments on commit 0db1890

Please sign in to comment.