Remove test constructors or mark as TestOnly

[bentsherman]

Fell down a rabbit hole while working on #5045 . Trying to remove test constructors where it's easy to do so, otherwise mark them as TestOnly. Would be good to get rid of them over time as they suggest to me that a class isn't easily testable.

[netlify]

✅ Deploy Preview for nextflow-docs-staging canceled.

Name	Link
🔨 Latest commit	130af1c
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-docs-staging/deploys/6814d2d50be8510008e762f8

[bentsherman]

@pditommaso do these changes look reasonable to you

[pditommaso]

@TestOnly is a nice way to mark those constructors, but why it requires changing so many tests? also seems there are some changes in charliecloud that looks unrelated.

[bentsherman]

I think I tried to remove test constructors in favor of using the real constructors as much as possible, where it didn't require too many changes to tests.

I think there was some issue recently where a test didn't catch a bug because it was using a test constructor and wasn't testing the behavior correctly. For this reason the test constructors seem like an anti-pattern, but in many cases they are convenient and it would be a hassle to remove them. So I just marked those with @testonly so that at least they are easier to track.

As for charliecloud, as far as I can tell that removed code is never used because registry is never set. I guess it's not related but I stumbled across it while making this PR.

[bentsherman]

@tom-seqera this PR should give you a nice survey of all the unit tests 😄

I created this PR because I encountered some error that wasn't caught by a unit test because the unit test was using a test constructor and bypassing the actual behavior

I have gone through the codebased and marked every test constructor as @TestOnly so that they are easy to find. I have also tried to remove them wherever it's easy to do so. Some of them are a bit trickier to remove, so I left them alone so as to not blow up this PR.

I know you have ideas to improve the tests at a higher level, especially around correctness. I hope this PR is a step in the right direction. If these changes make sense to you, please approve and I will merge it.

Let me know if you have any questions. If you find any of the test changes confusing, we can just revert them.

[bentsherman]

@tom-seqera all good now

[bentsherman]

Note, the failing nf-google tests are a separate issue and not related to this PR

[kdesnos]

@bentsherman
Changes done in the d4fadd4 commit break the unit tests on my computer, and when pushing new code to GitHub as well (see builds of PR #6040 ). With this code, the GoogleOpts class throws an exception at line 134 because the google project ID is missing, most likely because the "GOOGLE_APPLICATION_CREDENTIALS" is not defined for my runs on Github or on my personal computer.

As done in my PR code, I suggest adding the @Requires({System.getenv('GOOGLE_APPLICATION_CREDENTIALS')}) annotation to the 'should parse stdout and stderr' test within the BatchLoggingTest.

[bentsherman]

Thanks @kdesnos , I see that I missed this failure because I have google credentials defined in my local environment. I have submitted a patch.

It also seems to be affect some PRs but not all. No idea why 🤷‍♂️