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

Add Golden Copy Tests #1674

Draft
wants to merge 61 commits into
base: main
Choose a base branch
from
Draft

Add Golden Copy Tests #1674

wants to merge 61 commits into from

Conversation

pluckyswan
Copy link
Contributor

@pluckyswan pluckyswan commented Dec 23, 2024

Description

Added golden copy test to RS-E2E tests. The golden copy workflow will take the input files and submit them through the workflow. Then, it will fetch the output from Azure and compare them to the expected files. This utilizes the existing automated staging test submit workflow to send all files and adds a new run workflow specific to golden copy test.

Issue

#1600

Checklist

  • I have added tests to cover my changes
  • I have added logging where useful (with appropriate log level)
  • I have updated the documentation accordingly

Note: You may remove items that are not applicable

jherrflexion and others added 30 commits December 16, 2024 16:56
Co-Authored-By: Sylvie <[email protected]>
Co-Authored-By: Samuel Aquino <[email protected]>
Co-Authored-By: Samuel Aquino <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Samuel Aquino <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Samuel Aquino <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Samuel Aquino <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Samuel Aquino <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: saquino0827 <[email protected]>
Co-authored-by: Sylvie <[email protected]>
Co-authored-by: jcrichlake <[email protected]>
Co-authored-by: jcrichlake <[email protected]>
Co-authored-by: jcrichlake <[email protected]>
Co-authored-by: James Herr <[email protected]>
Co-authored-by: James Herr <[email protected]>
Co-authored-by: jcrichlake <[email protected]>
Co-authored-by: James Herr <[email protected]>
Co-authored-by: James Herr <[email protected]>
pluckyswan and others added 2 commits December 23, 2024 14:11
Co-authored-by: James Herr <[email protected]>
Co-authored-by: James Herr <[email protected]>
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1600 - Partially compliant

Fully compliant requirements:

  • Modify automated staging test workflows (submit and run).
  • Create test.
  • Add input and expected files.
  • Update/add README.

Not compliant requirements:

  • Every validation message from the UCSD implementation is tested as defined in the provided spreadsheet.
  • Golden copy tests failing do not impact whether or not existing RS e2e test will run, and vice versa.
  • Possible modify cron schedule if needed for golden copy.
  • Add new receivers in RS.
  • Test.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Environment Variable Dependency

The code heavily relies on the 'LOCAL_FILE_PATH' environment variable without a fallback, which could lead to runtime errors if not set.

        String files_path = System.getenv("LOCAL_FILE_PATH");
        if (files_path == null || files_path.isEmpty()) {
            throw new IllegalArgumentException("Environment variable LOCAL_FILE_PATH is not set");
        }

// when its pulled down and modify destinationName to be test folder specific
// possibly use a different receiver and filter on that

String testTypeAndSourceName = "Automated/" + sourceName;

Choose a reason for hiding this comment

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

Consider refactoring the blob naming and organizing logic into a separate method or class to improve modularity and testability. This change would help isolate the blob management logic, making the code cleaner and easier to maintain. [important]

for (filePair in matchedFiles) {
def actualFile = filePair.getKey()
def expectedFile = filePair.getValue()
if (!actualFile.equals(expectedFile)) {

Choose a reason for hiding this comment

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

Implement additional logging for each file comparison in the 'Compare files' test to provide more detailed feedback on which files are being compared and the result of each comparison. This enhancement would be beneficial for debugging and understanding test failures. [medium]

if (files_path.contains("GoldenCopy")) {
pathPrefix += datePrefix + "GoldenCopy/";
}

ListBlobsOptions options = new ListBlobsOptions().setPrefix(pathPrefix);

Choose a reason for hiding this comment

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

Add error handling for the Azure blob fetching process to manage exceptions and provide meaningful error messages if the fetching fails. This would improve the robustness of the file fetching mechanism. [important]

Copy link

PR Code Suggestions ✨

No code suggestions found for the PR.

@pluckyswan pluckyswan marked this pull request as draft December 27, 2024 16:46
@Override
public List<HL7FileStream> fetchFiles() {
String files_path = System.getenv("LOCAL_FILE_PATH");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder to add instructions to the readme on how to set up the LOCAL_FILE_PATH env var. You could also consider adding it to generate_env.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize this changes are in AzureBlobFileFetcher. This class is meant only for blobs in Azure. To grab local files, the correct one is LocalFileFetcher

jherrflexion and others added 6 commits December 27, 2024 11:05
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Jeremy Rosenfeld <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Jeremy Rosenfeld <[email protected]>
Co-Authored-By: Basilio Bogado <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Jeremy Rosenfeld <[email protected]>
Co-Authored-By: Basilio Bogado <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Jeremy Rosenfeld <[email protected]>
Co-Authored-By: Basilio Bogado <[email protected]>
Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: Luis Pabon <[email protected]>
Co-Authored-By: Jeremy Rosenfeld <[email protected]>
Co-Authored-By: Basilio Bogado <[email protected]>
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.

3 participants