Skip to content

Conversation

@alb3rtobr
Copy link
Contributor

StartMemberUtils contains utility methods used by StartLocatorCommand & StartServerCommand.

One of them is resolveWorkingDir which returns the working directory to be used by the member. Part of the logic of this procedure is placed outside the function:

workingDirectory = StartMemberUtils.resolveWorkingDir(workingDirectory == null ? null : new File(workingDirectory), new File(memberName));

This causes that tests of that function need to create an instance of StartLocatorCommand & StartServerCommand classes.
And this has two negative consequences:

  • When a new startup parameter is added to locators and/or server, the tests of resolveWorkingDir have to be modified.
  • Tests are duplicated: there is a test class for every class using resolveWorkingDir (StartLocatorCommandWorkingDirectoryTest & StartServerCommandWorkingDirectoryTest).

@onichols-pivotal
Copy link
Contributor

this looks ready to merge...if more changes coming, please convert to draft until it's ready

@alb3rtobr
Copy link
Contributor Author

this looks ready to merge...if more changes coming, please convert to draft until it's ready

The code is ready. Checks failed due to flaky tests. I have just added an empty commit to launch CI so I can merge this PR with checks passed.

@alb3rtobr alb3rtobr force-pushed the feature/GEODE-9226 branch from fdf54b0 to 39cae57 Compare May 4, 2021 14:05
@onichols-pivotal
Copy link
Contributor

this looks ready to merge...if more changes coming, please convert to draft until it's ready

The code is ready. Checks failed due to flaky tests. I have just added an empty commit to launch CI so I can merge this PR with checks passed.

Hey @alb3rtobr thanks for endeavoring to have all checks green before merging! This week however it's likely that DistributedTestOpenJDK8 may not readily pass. If the only failures are in DistributedTestOpenJDK8 and match known flaky tests identified by the latest mass test report, we can consider that good enough to merge.

@alb3rtobr alb3rtobr force-pushed the feature/GEODE-9226 branch from 39cae57 to d0d00f3 Compare May 4, 2021 17:59
@alb3rtobr
Copy link
Contributor Author

Thanks for the info @onichols-pivotal . While I was waiting for the green light, a conflict was introduced in develop so I have just rebased the PR. Hopefully it will be the last change. 🤞

@onichols-pivotal onichols-pivotal merged commit 97e191a into apache:develop May 5, 2021
@alb3rtobr alb3rtobr deleted the feature/GEODE-9226 branch May 5, 2021 20:18
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.

6 participants