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

Modified KffDataObjectHandler.hash(..) method so it explicitly replaces IBDO params it generates. #596

Conversation

drivenflywheel
Copy link
Collaborator

@drivenflywheel drivenflywheel commented Sep 21, 2023

A given object should have only one value for each type of hash we compute. With this change, the last one computed wins. Need to make sure that's the behavior we want.

Honestly, we should probably instead modify the behavior so that the hash method hashes a given payload at-most one time (so the computed hashes represent the data in its original form), with any subsequent calls being no-op'ed.

Copy link
Collaborator

@fbruton fbruton left a comment

Choose a reason for hiding this comment

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

Rebased and observed the following errors:

[ERROR]   IBaseDataObjectDiffHelperTest.testDiffCurrentForm:137 » ExceptionInInitializer
[ERROR]   UnixFilePlaceTest>IdentificationTest.testIdentificationPlace:61 »  java.lang.reflect.InvocationTargetException
[ERROR]   KffDataObjectHandlerTest.setUp:60 »  java.lang.reflect.InvocationTargetException
[ERROR]   KffHashPlaceTest.testProcess:24 » IllegalState No config directory specified

@drivenflywheel
Copy link
Collaborator Author

drivenflywheel commented Sep 25, 2023

Rebased and observed the following errors:

[ERROR]   IBaseDataObjectDiffHelperTest.testDiffCurrentForm:137 » ExceptionInInitializer
[ERROR]   UnixFilePlaceTest>IdentificationTest.testIdentificationPlace:61 »  java.lang.reflect.InvocationTargetException
[ERROR]   KffDataObjectHandlerTest.setUp:60 »  java.lang.reflect.InvocationTargetException
[ERROR]   KffHashPlaceTest.testProcess:24 » IllegalState No config directory specified

Run it again. That's an intermittent-but-recurring issue unrelated to this change. Since the order of unit test execution is randomized, it might indicate that the codebase has one or more unit test classes that don't extend Emissary's UnitTest class. I don't recall the details exactly, but I believe there's a order-of-operations issue that interferes with the initialization of tests executed after one of these "non-confirming" tests. Symptoms suggest contamination of system properties or the PROJECT_BASE environment variable

@dev-mlb
Copy link
Collaborator

dev-mlb commented Sep 26, 2023

Rebased and observed the following errors:

[ERROR]   IBaseDataObjectDiffHelperTest.testDiffCurrentForm:137 » ExceptionInInitializer
[ERROR]   UnixFilePlaceTest>IdentificationTest.testIdentificationPlace:61 »  java.lang.reflect.InvocationTargetException
[ERROR]   KffDataObjectHandlerTest.setUp:60 »  java.lang.reflect.InvocationTargetException
[ERROR]   KffHashPlaceTest.testProcess:24 » IllegalState No config directory specified

Run it again. That's an intermittent-but-recurring issue unrelated to this change. Since the order of unit test execution is randomized, it might indicate that the codebase has one or more unit test classes that don't extend Emissary's UnitTest class. I don't recall the details exactly, but I believe there's a order-of-operations issue that interferes with the initialization of tests executed after one of these "non-confirming" tests. Symptoms suggest contamination of system properties or the PROJECT_BASE environment variable

Ugh...I thought I had gotten all of those at one point. This should fix it: #597

@jpdahlke jpdahlke modified the milestones: v8.0.0-M8, v8.0.0-M9 Oct 2, 2023
@jpdahlke jpdahlke added the priority This has mirrored work driving the need label Oct 6, 2023
@jpdahlke jpdahlke requested a review from fbruton October 6, 2023 15:59
@jpdahlke jpdahlke dismissed fbruton’s stale review October 6, 2023 18:52

addressed comment

@jpdahlke jpdahlke merged commit 76a4659 into NationalSecurityAgency:master Oct 11, 2023
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority This has mirrored work driving the need
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants