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

ForEachTest resource #331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amumurst
Copy link

@amumurst amumurst commented Jul 7, 2021

Add IOForEachSuite as a way to acquire a new instance of a resource for each test in a suite.

Combining this with global makes it possible to do things that I have not seen in other test frameworks, for instance having the global scope setting up a Database provider and shutting it safely down after all tests are done, but still allowing tests to be written across multiple files, without dependencies on each other and with every test getting a fresh database.

I have tried to implement and test this pattern in UniqueResourceSuite and SharedResourceSuite.

If this is something you want to move forward with I would be happy to look at adding it to the docs.

…ansforming it and using the transformed value individually for each of the individual tests
@CLAassistant
Copy link

CLAassistant commented Jul 7, 2021

CLA assistant check
All committers have signed the CLA.

)


private[weaver] def getTests(args: List[String]) = {
Copy link
Author

Choose a reason for hiding this comment

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

This stuff is just abstracted back up to the common base trait between forEach and standard suite, no changes made.

@Baccata
Copy link
Contributor

Baccata commented Jul 7, 2021

@amumurst hi, and thanks for the contribution ! This feature had been requested by several people and was on my backlog, so the PR makes me happy.

I'm gonna take some time (probably tomorrow) to give it a proper read and reflect about the implications of your implementation, but at the very least I can tell you that there is interest 😄

@@ -58,10 +58,9 @@ abstract class RunnableSuite[F[_]] extends EffectSuite[F] {
effectCompat.sync(run(args)(outcome => effectCompat.effect.delay(report(outcome))))
}

abstract class MutableFSuite[F[_]] extends RunnableSuite[F] {
abstract class MutableBaseFSuite[F[_]] extends RunnableSuite[F] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so I think you're on the right track, however what I'd like to see is this :

trait MutableBaseFSuite[F[_]] {
    type Res 
    // PerTestRes would be replace Res in the "test" methods 
    type PerTestRes 

    def sharedResource: Resource[F, Res] 
    def perTestResource(res: Res) : Resource[F, PerTestRes] 
}

and then MutableFSuite would become :

trait MutableFSuite[F[_]] extends MutableBaseFSuite[F] {
   type PerTestRes = Res
   final def perTestResource(res: Res) : Resource[F, Res] = Resource.pure(res)  
}

The reason for doing it this way is that :

  1. it works for your usecase
  2. people can still share resources a little bit more granularly than your current version, which is "shared XOR per test", whereas this version would be "shared OR per test"
  3. it works on the javascript backend where cross-suite sharing is not possible, but per suite sharing is possible
  4. You can still have your MutableForEachSuite in userland :
trait MutableForEachSuite[F[_]] extends MutableBaseFSuite[F] {
   type Res = Unit 
   def uniqueResource : Resource[F, PerTestRes]
   final def perTestResource(res: Unit) : Resource[F, PerTestRes] = uniqueResource
}  

Copy link
Contributor

Choose a reason for hiding this comment

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

@amumurst, just to touch base : what do you think about the suggestion above ?

@vladimir-popov
Copy link

Any plans to bring this feature to the project?

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.

4 participants