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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

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
@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.

@jpdahlke jpdahlke requested a review from dev-mlb October 13, 2022 01:31
@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
Sam Bishop and others added 4 commits February 14, 2024 09:32
…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
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
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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Change will go out in a major version increase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants