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

refactor(artifact): moved artifacts (groovy) unit tests to artifacts (java) #1258

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

aman-agrawal
Copy link
Contributor

@aman-agrawal aman-agrawal commented May 31, 2024

  • moved ArtifactExtractorSpec groovy tests to ArtifactExtractorTest java tests
  • Newly added TCs are passing in ArtifactExtractorTest
  • deleted ArtifactExtractorSpec
  • changed parseCustomFormat() from private to package-private (default) to access it in testParseCustomFormatCorrectlyParsesBooleansAndStrings() & added @VisibleForTesting

--

  • moved ArtifactServiceSpec groovy tests to ArtifactServiceTest java tests
  • Newly added TCs are passing in ArtifactServiceTest
  • deleted ArtifactServiceSpec
  • ArtifactServiceSpec method "service finds artifact versions"() called getArtifactVersions(String, String, List<String>)
    To call same method from ArtifactServiceTest and to resolve the ambiguity issue between the overloaded methods, typecasted null with type List<String>.

@aman-agrawal aman-agrawal marked this pull request as ready for review June 1, 2024 01:15
@aman-agrawal aman-agrawal changed the title refactor(artifact_extractor): moved ArtifactExtractorSpec (groovy) unit tests to ArtifactExtractorTest (java) refactor(artifact): moved artifacts (groovy) unit tests to artifacts (java) Jun 1, 2024
@@ -37,7 +37,8 @@ public interface ArtifactService {
default List<String> getArtifactVersions(
@Nonnull String type, @Nonnull String name, String releaseStatus) {
List<String> releaseStatuses = new ArrayList<>();
releaseStatuses.add(releaseStatus);
if (releaseStatus == null) releaseStatuses = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more than a refactor. Can we leave the logic changes for a subsequent PR please? It also seems like a strange line of code to add...does it do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change reverted as this method is not called from unit test.
ArtifactServiceSpec method "service finds artifact versions"() called getArtifactVersions(String, String, List<String>)
To call same method from ArtifactServiceTest and to resolve the ambiguity issue between the overloaded methods, typecasted null with type List<String> in serviceFindsArtifactVersions()

@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Jun 11, 2024
@mergify mergify bot added the auto merged label Jun 11, 2024
@mergify mergify bot merged commit c05e0e0 into spinnaker:master Jun 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants