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

Rename TestTool to LarvaTool, to be more specific #6378

Merged
merged 7 commits into from
Mar 2, 2024

Conversation

jkosternl
Copy link
Contributor

  • and be less confusing since Ladybug is a TestTool too

* and be less confusing since Ladybug is a TestTool too
core/src/main/resources/log4j4ibis.xml Outdated Show resolved Hide resolved
core/src/main/resources/log4j4ibis.xml Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@ scenario.active=${active.fxf}

include = ../fxf.properties

fxffs.test.className=org.frankframework.testtool.FileSender
fxffs.test.className=org.frankframework.larva.FileSender
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch, I forgot about this.
This breaks backwards compatibility with existing test-scenarios that users have.

For the package-renaming of 8.0 we made a "fix" for this in the Larva property loader. I'm afraid we'll need to extend that, so that it also rewrites property-values starting with "org.frankframework.testtool" or "nl.nn.adapterframework.testtool" into org.frankframework.larva -- see the method rewriteClassName() in the Larva class!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I didn't think about others having Larva tests too. I'll add that to that 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.

Done, quicker than I thought 👍🏻 Hopefully this is Ok.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is it possible to revert some properties so we can test/ensure this works?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to revert some properties so we can test/ensure this works?

Dat was volgens mij in deze commit gedaan:

f6223ba

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, in the Base64Pipe.
@nielsm5, if the rest is Ok, can you merge it? When I have some tussenuurtjes Monday, I can do a next refactor piece on the Larva code base.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Ah, yea sorry... You don't see reverted changes in the pr overview 😶‍🌫️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, of course, no problem. Thanks for merging 👍🏻

* apply other feedback on PR from Tim
Copy link
Contributor

@tnleeuw tnleeuw left a comment

Choose a reason for hiding this comment

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

Sorry to request yet more changes now :-(

Copy link

sonarcloud bot commented Feb 29, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
6 Security Hotspots
1.6% Coverage on New Code (required ≥ 65%)
3.3% Duplication on New Code (required ≤ 3%)
E Reliability Rating on New Code (required ≥ A)
E Security Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkosternl Can you please take a look at this file? It looks to be an empty file, even in 7.9 already!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do that next week, in a next PR in the Larva space.. I have some upcoming improvements/refactors to go 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

The file appears to have been committed as empty file in 2019 as part of a large PR, and have lived on unchanged since then... 😬
No haste in removing 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.

Ah 😄

@nielsm5 nielsm5 merged commit 0b2b9ac into master Mar 2, 2024
14 of 17 checks passed
@nielsm5 nielsm5 deleted the chore/renameTestTool branch March 2, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants