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

Refactor TestVerifier logic #187

Open
lordofthejars opened this issue Sep 22, 2017 · 4 comments
Open

Refactor TestVerifier logic #187

lordofthejars opened this issue Sep 22, 2017 · 4 comments

Comments

@lordofthejars
Copy link
Member

Issue Overview

Currently TestVerifier class verifies if a file coming from SCM is a test by checking if it is a java file (ends with .java), if so it extracts the class name and then checks against the list of detected tests by surefire. If test class is in the list, then this is considered a test.

This works for Java projects but not for other JVM languages like Groovy or Scala and also does not work in case of wanting to create a meta decision related to some specific file (i.e user modifies persistence.xml).

For this reason this part must be refactored to a more extendible way like following a handle chain/compose pattern where each of the strategy can decide/filter what kind of files are important for him.

Expected Behaviour

Allow strategies to filter the kind of files they can use.

Current Behaviour

Smart Testing only works for .java files

@MatousJobanek
Copy link
Contributor

One technical comment regarding to the code of AffectedTestsDetector.getTests and the method TestVerifier.isCore
the method returns just a negation of the check if the class is a test class. However, in the provider here we create the TestVerifier saying that test classes are only those ones that are in the set of the tests to run: testsToRun.getClassByName(className) != null

In other words, consider test suite containing two test classes:

TestSomeObject.java
AnotherObjectTest.java

and inclusion: **/*Test.java
then the AffectedTestsDetector logic should take the first test class TestSomeObject.java as a business class.

Are you aware of it? As a side effect, the affected method should also work on CustomAssertion classes we were talking about a few days ago...

@lordofthejars
Copy link
Member Author

@MatousJobanek yes but at this point we cannot do more. We need to rewrite how to detect that a test is a test, and from surefire point of view TestSomeObject.java is not a test.

@MatousJobanek
Copy link
Contributor

Of course that from the surefire point of view it is not a test. But surefire doesn't use the non-test classes for some additional analysis as we do.
My question was mainly about if we are aware of it and if it should really work in the same way as I have written above - when I change some CustomAssertion class then the affected test classes are executed.
I have tried it and it seems that it doesn't work in that way - the test class using some CustomAssertion class that has been changed is not executed...

@lordofthejars
Copy link
Member Author

Maybe it will be worth to debug and see what's happening to understand why they are not executed, because I think it will have sense that they are executed.

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

No branches or pull requests

2 participants