Skip to content

Split the MiiSample classes to ItMiiWlsAuxiliaryImageSample and ItMiiFmwAuxiliaryImageSample to seedup test execution in parallel #2455

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 20, 2021
Merged

Split the MiiSample classes to ItMiiWlsAuxiliaryImageSample and ItMiiFmwAuxiliaryImageSample to seedup test execution in parallel #2455

merged 6 commits into from
Jul 20, 2021

Conversation

hzhao-github
Copy link
Member

@hzhao-github hzhao-github commented Jul 13, 2021 •
edited
Loading

@rjeberhard
Copy link
Member

What is the "Cm" in ItMiiWlsCmSamples?

@rjeberhard
Copy link
Member

What is the "Cm" in ItMiiWlsCmSamples?

Despite the title of your PR, you do seem to have updated all of the files with the new name :)

@hzhao-github hzhao-github changed the title Split the MiiSample classes to MiiWlsCmSamples and MiiFmwCmSamples to seedup test execution in parallel Split the MiiSample classes to ItMiiWlsAuxiliaryImageSample and ItMiiFmwAuxiliaryImageSample to seedup test execution in parallel Jul 13, 2021
vanajamukkara
vanajamukkara suggested changes Jul 13, 2021
View reviewed changes
integration-tests/src/test/java/oracle/weblogic/kubernetes/ItMiiWlsAuxiliaryImageSample.java Outdated
@@ -0,0 +1,325 @@
// Copyright (c) 2020, 2021, Oracle and/or its affiliates.
Copy link
Member

Choose a reason for hiding this comment

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

should be 2021

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

integration-tests/src/test/java/oracle/weblogic/kubernetes/ItMiiWlsAuxiliaryImageSample.java Outdated
import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* Tests to verify MII sample.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Tests to verify MII sample.
* Tests to verify MII sample using Auxiliary Image.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

integration-tests/src/test/java/oracle/weblogic/kubernetes/ItMiiFmwAuxiliaryImageSample.java Outdated
@@ -0,0 +1,385 @@
// Copyright (c) 2020, 2021, Oracle and/or its affiliates.
Copy link
Member

Choose a reason for hiding this comment

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

2021

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

integration-tests/src/test/java/oracle/weblogic/kubernetes/ItMiiFmwAuxiliaryImageSample.java Outdated
}
}

private static void assertImageExistsAndPushIfNeeded() {
Copy link
Member

Choose a reason for hiding this comment

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

there is duplicated code in all three test classes, can you move them out of the It class and make calls?

Copy link

Choose a reason for hiding this comment

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

+1

Copy link

Choose a reason for hiding this comment

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

This method is not the only duplicate code. It looks like constants, DomainType, initAll, exectTest*, callFmw*, have all bean duplicated.

Suggestion: create one class that contains the complete implementation and call it ITMiiSampleHelper or somesuch, then create four very similar small test suite classes 'ItMIISample[WLS|JRF][Main|Aux]Image' that (a) set a modal value on the helper class to change its behavior between WLS|JRF and Main|Aux, and (b) call the helper tests. The only difference between the four need only be description text, class name, and the setting of the modal...

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

Copy link

@tbarnes-us tbarnes-us Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks pretty nice. Just a little more to go in order to complete the refactoring: in my opinion, there's significant remaining duplicate code between the four tests' 'init' and 'tearDown' methods. These can be simplified to invoke a 'universal' init and tearDown in the new helper class. Once this duplicate code is factored out, then most of the constants and imports in the four tests no longer need to be defined (they can be local to the helper).

Also, you can add a auxiliaryImageEnabled() method that returns true or false - and use it to replace repeat usage of String useAuxiliaryImage = (getImageType().equalsIgnoreCase(ImageType.AUX.toString())) ? "true" : "false";

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

@hzhao-github hzhao-github marked this pull request as ready for review July 14, 2021 18:37
@hzhao-github hzhao-github removed the request for review from tbarnes-us July 15, 2021 18:46
tbarnes-us
tbarnes-us approved these changes Jul 19, 2021
View reviewed changes
Copy link

@tbarnes-us tbarnes-us 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!
I have some remaining minor comments - up to you if you want to follow up on them.

anpanigr
anpanigr approved these changes Jul 19, 2021
View reviewed changes
Copy link
Member

@anpanigr anpanigr left a comment

Choose a reason for hiding this comment

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

LGTM

@tbarnes-us
Copy link

General comment: Still approved, but note that the review comments state that you've made the suggested remaining minor changes. The problem is that I don't see them. Perhaps a forgotten push?

tbarnes-us
tbarnes-us reviewed Jul 20, 2021
View reviewed changes
integration-tests/src/test/java/oracle/weblogic/kubernetes/utils/ItMiiSampleHelper.java
final String baseImageNameKey = "BASE_IMAGE_NAME";
envMap.remove(baseImageNameKey);
execTestScriptAndAssertSuccess(domainType, args, errString);
envMap.put(baseImageNameKey, WEBLOGIC_IMAGE_NAME);
Copy link

Choose a reason for hiding this comment

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

minor: This looks like a refactoring issue. If, sometime in the future, the suite was changed so that this test was called from a different location and this is not fixed, then the test would potentially break the next test. The code shouldn't force WEBLOGIC_IMAGE_NAME -- instead, the code should restore the image that was originally in the envMap, similar to the original code, and it should ideally do this in a finally.

@rjeberhard rjeberhard merged commit bb41763 into main Jul 20, 2021
@rjeberhard rjeberhard deleted the split-itmiisample branch January 31, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbarnes-us tbarnes-us tbarnes-us approved these changes

@anpanigr anpanigr anpanigr approved these changes

@vanajamukkara vanajamukkara Awaiting requested review from vanajamukkara

@rjeberhard rjeberhard Awaiting requested review from rjeberhard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hzhao-github @rjeberhard @tbarnes-us @anpanigr @vanajamukkara