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

ARTEMIS-3576 Fix toString methods throwing exceptions #3865

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

brusdev
Copy link
Member

@brusdev brusdev commented Dec 1, 2021

No description provided.

@gemmellr
Copy link
Member

gemmellr commented Dec 1, 2021

Its nice to see the runtime has been reduced from ~30 seconds to 'only' ~5 seconds in this version of the PR. Thats still a significant chunk of time I dont think this is worth spending on each build for this, the classes changed should have specific tests of their behaviour taking milliseconds rather than relying on something like this, however it is at least less egregious with the further 25 seconds knocked off.

If we are going to have this it could at least produce the cleanest output possible to begin with though. Currently the output is still showing stacktraces for a couple of silly cases:

  • One of the same general cases many of the changes here are for, of the test causing an NPE on something thats reasonably expected to never to be null. Its just that JournalFileImpl is catching the NPE and logging it in this case, meaning it doesnt fail the test. As with the others it should just be enforced.
  • Its also showing up a finalizer log for ClientSessionFactoryImpl...which feels pointless given it doesnt actually seem to have a toString() method impl at all. Given it takes lots of constructor args, I'd guess one of them can be enforced on to shut it up.

@@ -238,6 +238,8 @@ public ClientSessionFactoryImpl(final ServerLocatorInternal serverLocator,
if (connectorConfig.getB() != null) {
this.backupConnectorConfig = connectorConfig.getB();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@brusdev I will send a PR removing this right now :)

that way you rebase without this...

Copy link
Contributor

Choose a reason for hiding this comment

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

here #3872

Copy link
Member Author

Choose a reason for hiding this comment

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

great thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants