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

Improved ExtractionTest Validation of Attachment and Extracted Children Expected vs Found Count #188

Merged

Conversation

sambish5
Copy link
Collaborator

@sambish5 sambish5 commented Dec 16, 2021

This is in reference to NationalSecurityAgency/burrito-grande#884.
Improved the ExtractionTest to further checks on Attachment and Extracted Children counts. Includes new tests in TestExtractionTest to verify count check works.

src/main/java/emissary/test/core/ExtractionTest.java Outdated Show resolved Hide resolved
src/main/java/emissary/test/core/ExtractionTest.java Outdated Show resolved Hide resolved
src/main/java/emissary/test/core/ExtractionTest.java Outdated Show resolved Hide resolved
src/main/java/emissary/test/core/ExtractionTest.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@dev-mlb dev-mlb left a comment

Choose a reason for hiding this comment

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

lgtm - just need to fix the import statements

src/main/java/emissary/test/core/ExtractionTest.java Outdated Show resolved Hide resolved
dev-mlb
dev-mlb previously approved these changes Feb 17, 2022
@jpdahlke jpdahlke added this to the v8.0.0 milestone Apr 8, 2022
@sambish5 sambish5 force-pushed the emissary-extractiontest-884 branch from ddcd129 to 4419433 Compare August 19, 2022 15:47
@drivenflywheel
Copy link
Collaborator

This seems way more burdensome than the original ask, which was "don't allow the XML to have more child attachment or extractedRecord elements than specified in the numAttachments / extractionCount elements".

As currently implemented, we can no longer assert the numAttachments or extractionCount without creating a child element for every one of those children. That could cause an enormous burden for downstream integration. and it will actively hinder extraction tests for some packaged file processing. If nothing else, the ExtractionTest class should provide the ability to disable this check on a class-by-class or test-by-test basis so downstream projects can integrate this version of emissary without requiring a huge integration PR with thousands of lines of new XML.

@sambish5 sambish5 force-pushed the emissary-extractiontest-884 branch from 0458b20 to d0e5d9a Compare October 12, 2022 20:08
@jpdahlke jpdahlke requested a review from dev-mlb October 13, 2022 01:31
@drivenflywheel drivenflywheel self-requested a review December 9, 2022 22:22
@cfkoehler cfkoehler added the major Change will go out in a major version increase label Dec 15, 2022
@sambish5 sambish5 added the rebase Things have happened and now a rebase is needed label Sep 12, 2023
@sambish5 sambish5 force-pushed the emissary-extractiontest-884 branch from d0e5d9a to 85c96bb Compare February 16, 2024 18:47
@sambish5
Copy link
Collaborator Author

@drivenflywheel @dev-mlb some major updates within this. Instead of having it fail if <numAttachments> and <extractCount> do not match <att#> and <extract#>, it will now just log the non-matching counts. I currently have it written so even if numAttachments/extractCount are not specified, the check still occurs (this can easily be moved to within the if > -1 parts if we only want it to check when those are specified, but I figured we'd want it to always be specified if there are att or extract in the xml).

If you guys would prefer that method however, I can move it inside of those ifs, and in the future if we want to build back out the functionality to fail if the counts do not match, we can.

@sambish5 sambish5 removed the rebase Things have happened and now a rebase is needed label Feb 16, 2024
dev-mlb
dev-mlb previously approved these changes Feb 16, 2024
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Last changes? 🤞

@sambish5 sambish5 requested a review from drivenflywheel March 12, 2024 18:44
@sambish5
Copy link
Collaborator Author

@drivenflywheel made the suggested changes, had to make some slight changes to the assertion things to make it work like how it did when it was just logging

drivenflywheel
drivenflywheel previously approved these changes Mar 12, 2024
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Looks good. Sorry for the high number cycles.

@jpdahlke jpdahlke removed this from the v8.0.0 milestone Apr 2, 2024
@sambish5 sambish5 added the rebase Things have happened and now a rebase is needed label Sep 10, 2024
Sam Bishop added 2 commits September 19, 2024 10:11
…en Expected vs Found Count. Added 3 more tests to TestExtractionTest to further tests.
…ld be covered now and should throw the correct error in each case. Added more tests to verify errors being thrown in each case. Removed two of the previous testing xmls, created a new, simpler xml for testing
@sambish5 sambish5 removed the rebase Things have happened and now a rebase is needed label Sep 19, 2024
@sambish5
Copy link
Collaborator Author

rebased and made some minor updates to just include the test name when validation finds expected vs found count not equal on assertion, hopefully this can close this PR 🤞

@sambish5 sambish5 added the integration A breaking change where effort will be required downstream to resolve label Sep 19, 2024
drivenflywheel
drivenflywheel previously approved these changes Sep 23, 2024
dev-mlb
dev-mlb previously approved these changes Sep 23, 2024
@jpdahlke jpdahlke added enhancement An enhancement or update to an existing feature and removed major Change will go out in a major version increase labels Sep 25, 2024
@jpdahlke jpdahlke added this to the v8.13.0 milestone Sep 25, 2024
@sambish5 sambish5 dismissed stale reviews from dev-mlb and drivenflywheel via 4e39266 September 26, 2024 20:21
@sambish5
Copy link
Collaborator Author

Changes made by request of @jpdahlke
Overview of changes:

  • no longer fails if <numAttachments> & <extractCount> DNE <att#> or <extract#> in answer file, now just logs differences.
  • Has a new priority system to make sure that counts in answer file match counts in payload:
    - Checks <numAttachments>/<extractCount> if present in answer file against count from payload
    - If those are not present, check against <att#>/<extract#> if present
    - If neither are present, and there are atts or extracts in the payload, FAIL the test, saying <numAttachments>/<extractCount> matching payload count needs to be in answer file

Copy link
Collaborator

@jpdahlke jpdahlke left a comment

Choose a reason for hiding this comment

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

I think this update makes sense given our integration experiences.

@jpdahlke jpdahlke self-requested a review September 26, 2024 21:48
@jpdahlke jpdahlke merged commit e7d552d into NationalSecurityAgency:main Sep 30, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or update to an existing feature integration A breaking change where effort will be required downstream to resolve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants