[terra-functional-testing] Fixing Nexus screenshot bugs (#839)

* fixes for upload/download to deal with multinode jenkins runs * final changes * accidently removed logger line, readding it * fixing lint and adding jest tests * adding changelog and fixing linter errors * fixing falsy function call for making reference name * moving off sync functions in async oncomplete * fixing lint error * Used better testing methods and changed wdioterraservice to test new async calls * Update CHANGELOG.md --------- Co-authored-by: Saad Adnan <[email protected]>
cerner · Jan 23, 2024 · 59131a2 · 59131a2
1 parent 9751a95
commit 59131a2
Show file tree

Hide file tree

Showing 8 changed files with 543 additions and 189 deletions.
diff --git a/packages/terra-functional-testing/CHANGELOG.md b/packages/terra-functional-testing/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 ## Unreleased
 
+* Changed
+  * Updated upload and download logic in Nexus screenshots based on current locale, theme, browser, and formFactor.
+  * Updated PR comment logic in Nexus screenshots.
+  * Updated Nexus mismatch warning to display testname and to be outputted via Logger.
+
 ## 4.5.0 - (December 11, 2023)
 
 * Changed

diff --git a/packages/terra-functional-testing/src/commands/expect/toMatchReference.js b/packages/terra-functional-testing/src/commands/expect/toMatchReference.js
@@ -1,4 +1,10 @@
+const path = require('path');
+const fs = require('fs-extra');
+const { Logger } = require('@cerner/terra-cli');
+const getOutputDir = require('../../reporters/spec-reporter/get-output-dir');
 const { BUILD_BRANCH, BUILD_TYPE } = require('../../constants');
+
+const logger = new Logger({ prefix: '[terra-functional-testing:toMatchReference]' });
 /**
  * An assertion method to be paired with Visual Regression Service to assert each screenshot is within
  * the mismatch tolerance and are the same size.
@@ -11,7 +17,7 @@ const { BUILD_BRANCH, BUILD_TYPE } = require('../../constants');
  * @param {boolean} screenshot.screenshotWasUpdated - If the reference screenshot was updated with the latest captured screenshot.
  * @returns {Object} - An object that indicates if the assertion passed or failed with a message.
  */
-function toMatchReference(screenshot) {
+function toMatchReference(screenshot, testName) {
   const {
     isNewScreenshot,
     isSameDimensions,
@@ -41,11 +47,19 @@ function toMatchReference(screenshot) {
   if (global.Terra.serviceOptions.useRemoteReferenceScreenshots && !pass
     && (global.Terra.serviceOptions.buildBranch.match(BUILD_BRANCH.pullRequest) || global.Terra.serviceOptions.buildType === BUILD_TYPE.branchEventCause)) {
     pass = true;
-    process.env.SCREENSHOT_MISMATCH_CHECK = true;
-    if (message.length > 0) {
-      message = message.concat('\n');
+    const outputDir = getOutputDir();
+    const fileName = path.join(outputDir, 'ignored-mismatch.json');
+    // Create the output directory if it does not already exist.
+    if (!fs.existsSync(outputDir)) {
+      fs.mkdirSync(outputDir, { recursive: true });
+      // Since  output directory didn't exist file couldnt so just go ahead and make the file
+      fs.writeFileSync(fileName, JSON.stringify({ screenshotMismatched: true }, null, 2));
+    } else if (!fs.existsSync(fileName)) {
+      // If output directory exists but mismatch file has not been created create one
+      fs.writeFileSync(fileName, JSON.stringify({ screenshotMismatched: true }, null, 2));
     }
-    message = message.concat('Screenshot has changed and needs to be reviewed.');
+
+    logger.info(`Test: '${testName}' has a mismatch difference of ${misMatchPercentage}% and needs to be reviewed.`);
   }
 
   return {

diff --git a/packages/terra-functional-testing/src/commands/utils/ScreenshotRequestor.js b/packages/terra-functional-testing/src/commands/utils/ScreenshotRequestor.js
@@ -55,10 +55,11 @@ class ScreenshotRequestor {
 
   /**
    * Deletes the existing screenshots
+   * @param {string} referenceName - the name of the reference screenshots file to download
    */
-  async deleteExistingScreenshots() {
+  async deleteExistingScreenshots(referenceName) {
     const response = await fetch(
-      this.serviceUrl,
+      `${this.serviceUrl}${referenceName}/`,
       {
         method: 'DELETE',
         headers: {
@@ -80,6 +81,19 @@ class ScreenshotRequestor {
     fs.removeSync(archiveName);
   }
 
+  /**
+   * Makes string to return for the screenshots to save and download
+   * Naming convention is {theme}-{locale}-{browser}-{formfactor} and if provided string is empty does not add it.
+   * @param {string} locale - the locale to use when downloading
+   * @param {string} theme - the theme to use when downloading
+   * @param {string} formFactor - the formFactor to use when downloading
+   * @param {string} browser - the browser to use when uploading
+   */
+  static makeReferenceName(locale, theme, formFactor, browser) {
+    const referenceName = [theme, locale, browser, formFactor].filter((str) => str).join('-');
+    return referenceName;
+  }
+
   /**
    * Zips the latest screenshots.
    */
@@ -104,7 +118,7 @@ class ScreenshotRequestor {
 
     const archiveName = path.join(this.zipFilePath, 'latest.zip');
 
-    // Name the uploaded file reference.zip since the latest screenshots will now be used as the reference screenshots.
+    // Name the uploaded file the passed referenceName since the latest screenshots will now be used as the reference screenshots.
     archive.file(archiveName, { name: 'reference.zip' });
 
     await archive.finalize();
@@ -115,20 +129,21 @@ class ScreenshotRequestor {
 
   /**
    * Downloads the screenshots and unzip it to the reference screenshot directory defined by referenceScreenshotsPath.
+   * @param {string} referenceName - the name of the reference screenshots file to download
    */
-  async downloadScreenshots() {
+  async downloadScreenshots(referenceName) {
     let archiveUrl;
     let fetchOptions;
     if (this.serviceAuthHeader !== undefined) {
-      archiveUrl = `${this.serviceUrl}/reference.zip`;
+      archiveUrl = `${this.serviceUrl}${referenceName}/reference.zip`;
       fetchOptions = {
         method: 'GET',
         headers: {
           Authorization: this.serviceAuthHeader,
         },
       };
     } else {
-      archiveUrl = `${this.url}/reference.zip`;
+      archiveUrl = `${this.url}${referenceName}/reference.zip`;
       fetchOptions = {
         method: 'GET',
       };
@@ -139,7 +154,7 @@ class ScreenshotRequestor {
     );
 
     if (response.status === 404) {
-      logger.info(`No screenshots downloaded from ${this.url}. Either the URL is invalid or no screenshots were previously uploaded.`);
+      logger.info(`No screenshots downloaded from ${this.url}${referenceName}. Either the URL is invalid or no screenshots were previously uploaded.`);
       return;
     }
 
@@ -153,7 +168,7 @@ class ScreenshotRequestor {
         writeStream.on('finish', async () => {
           await extract('terra-wdio-screenshots.zip', { dir: this.referenceScreenshotsPath });
           fs.removeSync('terra-wdio-screenshots.zip');
-          logger.info(`Screenshots downloaded from ${this.url}`);
+          logger.info(`Screenshots downloaded from ${this.url}${referenceName}`);
           resolve();
         });
       } catch (error) {
@@ -167,12 +182,13 @@ class ScreenshotRequestor {
   /**
    * Uploads the site zip contained in memoryStream
    * @param {MemoryStream} memoryStream - the MemoryStream to use when uploading
+   * @param {string} referenceName - the name of the reference zip to use when uploading
    */
-  async uploadScreenshots(memoryStream) {
+  async uploadScreenshots(memoryStream, referenceName) {
     const formData = new FormData();
     formData.append('file', memoryStream, { filename: 'reference.zip', knownLength: memoryStream.length });
     const response = await fetch(
-      this.serviceUrl,
+      `${this.serviceUrl}${referenceName}/`,
       {
         method: 'PUT',
         headers: {
@@ -183,22 +199,32 @@ class ScreenshotRequestor {
     );
     ScreenshotRequestor.checkStatus(response);
 
-    logger.info(`Screenshots are uploaded to ${this.url}`);
+    logger.info(`Screenshots are uploaded to ${this.url}${referenceName}`);
   }
 
   /**
    * Downloads the screenshots.
+   * @param {string} locale - the locale to use when downloading
+   * @param {string} theme - the theme to use when downloading
+   * @param {string} formFactor - the formFactor to use when downloading
+   * @param {string} browser - the browser to use when uploading
    */
-  async download() {
-    await this.downloadScreenshots();
+  async download(locale, theme, formFactor, browser) {
+    const referenceName = ScreenshotRequestor.makeReferenceName(locale, theme, formFactor, browser);
+    await this.downloadScreenshots(referenceName);
   }
 
   /**
    * Uploads the screenshots by deleting the existing screenshots, zipping the new ones, and uploading it
+   * @param {string} locale - the locale to use when uploading
+   * @param {string} theme - the theme to use when uploading
+   * @param {string} formFactor - the formFactor to use when uploading
+   * @param {string} browser - the browser to use when uploading
    */
-  async upload() {
+  async upload(locale, theme, formFactor, browser) {
+    const referenceName = ScreenshotRequestor.makeReferenceName(locale, theme, formFactor, browser);
     // Delete the existing screenshots from the remote repository because new screenshots will be uploaded.
-    await this.deleteExistingScreenshots();
+    await this.deleteExistingScreenshots(referenceName);
 
     // Zip up the existing latest screenshots
     await this.zipLatestScreenshots();
@@ -207,7 +233,7 @@ class ScreenshotRequestor {
     const memoryStream = await this.zipDirectoryToMemory();
 
     // Upload the screenshots to the remote repository
-    await this.uploadScreenshots(memoryStream);
+    await this.uploadScreenshots(memoryStream, referenceName);
 
     // The zipped latest screenshots can now be safely deleted.
     this.deleteZippedLatestScreenshots();

diff --git a/packages/terra-functional-testing/src/commands/validates/screenshot.js b/packages/terra-functional-testing/src/commands/validates/screenshot.js
@@ -22,7 +22,7 @@ const screenshot = (testName, options = {}) => {
 
   const screenshotResult = global.browser.checkElement(selector || global.Terra.serviceOptions.selector, wrappedOptions);
 
-  global.expect(screenshotResult).toMatchReference();
+  global.expect(screenshotResult).toMatchReference(testName);
 };
 
 module.exports = screenshot;
diff --git a/packages/terra-functional-testing/src/services/wdio-terra-service/WDIOTerraService.js b/packages/terra-functional-testing/src/services/wdio-terra-service/WDIOTerraService.js
@@ -2,6 +2,7 @@ const path = require('path');
 const expect = require('expect');
 const fs = require('fs-extra');
 const { SevereServiceError } = require('webdriverio');
+const getOutputDir = require('../../reporters/spec-reporter/get-output-dir');
 const { accessibility, element, screenshot } = require('../../commands/validates');
 const { toBeAccessible, toMatchReference } = require('../../commands/expect');
 const {
@@ -64,6 +65,10 @@ class WDIOTerraService {
       gitToken,
       issueNumber,
       useRemoteReferenceScreenshots,
+      locale,
+      theme,
+      formFactor,
+      browser,
     } = this.serviceOptions;
 
     if (!useRemoteReferenceScreenshots) {
@@ -90,7 +95,7 @@ class WDIOTerraService {
         screenshotConfig = this.getRemoteScreenshotConfiguration(this.screenshotsSites, buildBranch);
       }
       const screenshotRequestor = new ScreenshotRequestor(screenshotConfig.publishScreenshotConfiguration);
-      await screenshotRequestor.download();
+      await screenshotRequestor.download(locale, theme, formFactor, browser);
     } catch (error) {
       throw new SevereServiceError(error);
     }
@@ -211,9 +216,7 @@ class WDIOTerraService {
       ':warning: :bangbang: **WDIO MISMATCH**\n\n',
       `Check that screenshot change is intended at: ${buildUrl}\n\n`,
       'If screenshot change is intended, remote reference screenshots will be updated upon PR merge.\n',
-      'If screenshot change is unintended, please fix screenshot issues before PR merge to prevent them from being uploaded.\n\n',
-      'Note: This comment only appears the first time a screenshot mismatch is detected on a PR build, ',
-      'future builds will need to be checked for unintended screenshot mismatches.',
+      'If screenshot change is unintended, please fix screenshot issues before PR merge to prevent them from being uploaded.',
     ].join('');
 
     const comments = await issue.getComments();
@@ -230,12 +233,16 @@ class WDIOTerraService {
   async uploadBuildBranchScreenshots() {
     const {
       buildBranch,
+      locale,
+      theme,
+      formFactor,
+      browser,
     } = this.serviceOptions;
 
     try {
       const screenshotConfig = this.getRemoteScreenshotConfiguration(this.screenshotsSites, buildBranch);
       const screenshotRequestor = new ScreenshotRequestor(screenshotConfig.publishScreenshotConfiguration);
-      await screenshotRequestor.upload();
+      await screenshotRequestor.upload(locale, theme, formFactor, browser);
     } catch (err) {
       throw new SevereServiceError(err);
     }
@@ -256,14 +263,38 @@ class WDIOTerraService {
       return;
     }
 
+    const fileName = path.join(getOutputDir(), 'ignored-mismatch.json');
+
     try {
-      if (process.env.SCREENSHOT_MISMATCH_CHECK === 'true' && buildBranch.match(BUILD_BRANCH.pullRequest)) {
-        // We found a screenshot mismatch during our build of this PR branch.
-        await this.postMismatchWarningOnce();
-      } else if (!buildBranch.match(BUILD_BRANCH.pullRequest) && buildType === BUILD_TYPE.branchEventCause) {
-        // This non-PR branch is being merged or someone pushed code into it directly.
-        await this.uploadBuildBranchScreenshots();
-      }
+      // Check if the file exists in the current directory.
+      fs.access(fileName, fs.constants.F_OK, async (error) => {
+        try {
+          if (!error && buildBranch.match(BUILD_BRANCH.pullRequest)) {
+            // We found a screenshot mismatch during our build of this PR branch.
+            await this.postMismatchWarningOnce();
+            // Remove mismatch flag file after running
+            fs.unlink(fileName, (err) => {
+              if (err) throw err;
+            });
+          } else if (!buildBranch.match(BUILD_BRANCH.pullRequest) && buildType === BUILD_TYPE.branchEventCause) {
+            // This non-PR branch is being merged or someone pushed code into it directly.
+            await this.uploadBuildBranchScreenshots();
+            // Remove mismatch flag file after running if it exists
+            if (!error) {
+              fs.unlink(fileName, (err) => {
+                if (err) throw err;
+              });
+            }
+          }
+        } catch (err) {
+          // The service will stop only if a SevereServiceError is thrown.
+          if (err instanceof SevereServiceError) {
+            throw err;
+          }
+
+          throw new SevereServiceError(err);
+        }
+      });
     } catch (err) {
       // The service will stop only if a SevereServiceError is thrown.
       if (err instanceof SevereServiceError) {