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

Test resources are set up multiple times #661

Open
Rinzwind opened this issue Sep 12, 2024 · 9 comments · May be fixed by #662
Open

Test resources are set up multiple times #661

Rinzwind opened this issue Sep 12, 2024 · 9 comments · May be fixed by #662
Labels

Comments

@Rinzwind
Copy link
Contributor

Given a TestResource subclass DummyTestResource with #setUp implemented as showing 'Setting up DummyTestResource' on the transcript, after adding DummyTestResource to the resources for SCIAbstractLoadSpecTest, the message is shown five times, rather than just once, when running the smalltalkCI tests using Pharo64-13.

Reverting the change made to #runClasses:spec: for SCIPharoTestRunner in commit 7fed379 causes the message to only be shown once as expected (but also causes the test #testCustomTestSuite introduced in the same commit to fail).

@fniephaus
Copy link
Member

Thanks for filing this issue. Could one of you take a look please, @mattonem and @theseion?

@theseion
Copy link
Collaborator

@Rinzwind Why is your expectation that the message is only shown once? Resources are set up and torn down for every test suite and Dr. Test constructs one suite per test class. Ergo, you should see the message as many times as there are test classes that use the resource.

@Rinzwind
Copy link
Contributor Author

Because I expected it to use a single suite like #runClasses:named: for HDTestReport does do (at least in Pharo 13 build 225). That ignores overrides of #suiteClass though. In #runClasses:spec: for SCIPharoTestRunner the resources could be combined so that they only need to be set up once, while still keeping each class’s #suiteClass into account, by replacing the ‘do’ over the classes with the following:

resources := OrderedCollection new.
classes do: [ :class |
	| classSuite |
	classSuite := class suite.
	resources addAll: (classSuite resources reject: [ :resource | resources includes: resource ]).
	classSuite resources: #().
	suite addTest: classSuite ].
suite resources: resources.

@theseion
Copy link
Collaborator

That will prevent overrides of #suiteClass from working. I don't feel that this is an issue with SmalltalkCI but with SUnit in Pharo. TestSuite>>#tearDown sends #resetResources to TestResource, which resets all the resources of the suite. This completely defeats the idea that suites can be nested. IMO, nested suites must know that they are nested and leave tear down to the outer suite (this already works for #setUp, because TestResource>>#isAvailable returns true after the outer suite has run initialization).

@Rinzwind
Copy link
Contributor Author

I don’t quite follow how it would prevent overrides of #suiteClass from working. I tried replacing the ‘do’ in #runClasses:spec: for SCIPharoTestRunner. That allowed #testCustomTestSuite to still pass while causing the message of the test resource in my example to be shown only once instead of five times. I do agree though the framework should probably better take into account that suites can be composed when it comes to resources.

@theseion
Copy link
Collaborator

You're right, it does work with the code you posted, sorry. I had simply patched the code to mimic what HDTestReport does, which is to construct a single suite. I'll look into patching SmalltalkCI. Would you mind raising the issue with Pharo? Or would you rather I do that?

@Rinzwind
Copy link
Contributor Author

I opened Pharo issue #17113 about it.

theseion added a commit to theseion/smalltalkCI that referenced this issue Sep 17, 2024
@theseion theseion linked a pull request Sep 17, 2024 that will close this issue
@theseion
Copy link
Collaborator

I've opened a PR (#662). I'd appreciate your feedback @Rinzwind.

@Rinzwind
Copy link
Contributor Author

I added my feedback to the pull request.

theseion added a commit to theseion/smalltalkCI that referenced this issue Sep 19, 2024
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 a pull request may close this issue.

3 participants