Skip to content

Conversation


logger.info("Creating unique namespace for Domain2");
assertNotNull(namespaces.get(2), "Namespace list is null");
domain2Namespace = namespaces.get(2);
Copy link
Member

Choose a reason for hiding this comment

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

Why we need 2 domains in this usecases ? If needed please explain on the javadoc section of the test method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to make the tests independent from each other.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather prefer merge the usecase to avoid multiple domain boot if possible, you can make the Data Source uecase as additional common mount in addition to the existing test method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed test to have 1 domain

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

domain2Uid, miiCMImage3, domain2Namespace);
createDomainAndVerify(domain2Uid, domainCR, domain2Namespace,
adminServerPodName, managedServerPrefix, replicaCount);
podsWithTimeStamps = getPodsWithTimeStamps(adminServerPodName,
Copy link
Contributor

Choose a reason for hiding this comment

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

you can move this into patchDomainWithCMImageAndVerify method as that's where its used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

+ " in the new patched domain");

//get current timestamp before domain rolling restart to verify domain roll events
OffsetDateTime timestamp = now();
Copy link
Contributor

Choose a reason for hiding this comment

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

not used, you can remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

String.format("Rolling restart failed for domain %s in namespace %s", domainUid, domainNamespace));
}

private Map getPodsWithTimeStamps(String adminServerPodName, String managedServerPrefix,
Copy link
Contributor

Choose a reason for hiding this comment

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

have you checked if there are any utility methods? we may be doing this in multiple tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched to common util

int adminServiceNodePort
= getServiceNodePort(domain2Namespace, getExternalServicePodName(adminServerPodName), "default");
assertNotEquals(-1, adminServiceNodePort, "admin server default node port is not valid");
assertTrue(checkDS(adminServerPodName,domain2Namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check the methods in CommonTestUtils if you can use them - checkSystemResourceConfig and others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched to common util

@marinakog marinakog requested a review from vanajamukkara July 8, 2021 01:48
@marinakog
Copy link
Contributor Author

Copy link
Contributor

@vanajamukkara vanajamukkara left a comment

Choose a reason for hiding this comment

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

LG

@rjeberhard rjeberhard merged commit e27117c into main Jul 8, 2021
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.

4 participants