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

fix(catalog-generator): Fix Windows tests #1572

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Oct 13, 2024

Context

Currently, the catalog-generator tests are failing in Microsoft Windows due to the file separator.

This PR fixes it by using the underlying OS file separator instead.

@lordrip lordrip force-pushed the fix/catalog-tests-windows branch 2 times, most recently from d0313d6 to 1abc733 Compare October 13, 2024 17:11
@@ -84,7 +86,9 @@ void testGeneratorCalledWithCorrectParameters() {
File expectedFolder = new File(tempDir, "camel-main/4.8.0");
verify(builder, times(1)).withOutputDirectory(expectedFolder);

assertEquals(catalogDefinition.getFileName(), "camel-main/4.8.0/index.json");
String expectedFile = Stream.of("camel-main", "4.8.0", "index.json").reduce((a, b) -> a + File.separator + b).get();
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way to do this? A more Java way

Copy link
Member

Choose a reason for hiding this comment

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

i think it is more common to write it this way (at least for old timers):

"camel"+File.separator+"4.8.0"+File.separator+"index.json"

With more recent features of Java, I would look to something like: Paths.get("camel", "4.8.0", "index.json").toString()

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems Path.get() doesn't exist but Path.of() worked 👍

Copy link
Member

Choose a reason for hiding this comment

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

It is Paths with plural

Copy link
Member

Choose a reason for hiding this comment

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

Path.of() can do the trick too

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, plural 🤦‍♂️

Copy link

codecov bot commented Oct 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@c9f7a97). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1572   +/-   ##
=======================================
  Coverage        ?   80.00%           
  Complexity      ?      272           
=======================================
  Files           ?      276           
  Lines           ?     7838           
  Branches        ?     1535           
=======================================
  Hits            ?     6271           
  Misses          ?     1458           
  Partials        ?      109           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lordrip lordrip force-pushed the fix/catalog-tests-windows branch from 1abc733 to 8598e34 Compare October 14, 2024 14:06
Copy link

sonarcloud bot commented Oct 14, 2024

@lordrip lordrip merged commit 26963be into KaotoIO:main Oct 14, 2024
12 checks passed
@lordrip lordrip deleted the fix/catalog-tests-windows branch October 14, 2024 14:38
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.

2 participants