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

feat(#114): implements configuration file per module. #262

Merged
merged 20 commits into from
Dec 13, 2017

Conversation

dipak-pawar
Copy link
Contributor

Changes proposed in this pull request:

  • Supporting configuration file per module
  • Introduced inherit parameter in configuration file.
  • Updated documentation
  • Adds ftest in test-bed for multi-module project.
parent
 - config
   - smart-testing.yml 
   - api
   - impl-base
   - spi
     - smart-testing.yml
 - container
    - api
    - impl-base
 - smart-testing.yml 

Configuration file selection for the above example will be as follows:

  • config/api - parent/config/smart-testing.yml
  • config/impl-base - parent/config/smart-testing.yml
  • config/spi - parent/config/spi/smart-testing.yml
  • container/api - parent/smart-testing.yml
  • container/impl-base - parent/smart-testing.yml

Fixes #114

static Map<String, Object> getConfigParametersFromFile(Path filePath) {
if (!filePath.toFile().exists()) {
logger.warn(String.format("The configuration file %s is not exists.", filePath));
return new HashMap<>(0);
Copy link
Member

Choose a reason for hiding this comment

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

Are we mutating this map afterward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no we are not mutating this map afterwords

Copy link
Member

@bartoszmajsak bartoszmajsak Nov 22, 2017

Choose a reason for hiding this comment

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

Then maybe .emptyMap() would be better so we actually prevent from mutating it in the caller anyway (at least what is in the map, not the content itself)

Map<String, Object> yamlConfiguration = readConfiguration(configFile);
return parseConfiguration(yamlConfiguration);
final Configuration configuration = parseConfiguration(yamlConfiguration);
final ObjectMapper objectMapper = new ObjectMapper();
Copy link
Member

Choose a reason for hiding this comment

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

This can be a constant I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using it only once in ConfigurationLoader. I am not sure why do we really need to declare it as constant?

Copy link
Member

Choose a reason for hiding this comment

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

I misinterpreted it with Jackson object mapper object at first, so probably not. OTOH I don't feel like having mapToObject method with now a constructor and a call to readValue is really needed.

} catch (IOException e) {
throw new UncheckedIOException(e);
}
return readParseOverWriteConfiguration(path.toFile());
Copy link
Member

Choose a reason for hiding this comment

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

This method name makes me a bit concerned as we are doing 3 things at once. But also shows that this class as such is doing the same (3 responsibilities). Can you try to figure out how to clean it?

import static org.arquillian.smart.testing.configuration.ConfigurationLoader.SMART_TESTING_YML;
import static org.assertj.core.api.Assertions.assertThat;

public class ConfigurationOverWriteUsingInheretTest {
Copy link
Member

Choose a reason for hiding this comment

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

Inherit

import static org.assertj.core.api.Assertions.assertThat;

@Category(NotThreadSafe.class)
public class ConfigurationOverWriteUsingInheretWithSystemPropertyTest {
Copy link
Member

Choose a reason for hiding this comment

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

Inherit

@@ -63,16 +62,19 @@ public ProjectConfigurator withConfiguration(Configuration configuration) {
return this;
}

public ProjectConfigurator createConfigFile() {
public ProjectConfigurator withConfigFile() {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason behind this rename?

.withConfiguration(newConfiguration)
.withConfigFileIn("config/impl-base")
.withConfiguration(changedConfiguration)
.withConfigFileIn("junit/core")
Copy link
Member

Choose a reason for hiding this comment

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

This chain is a bit hard for me to follow. Can you explain what is the desired flow of action here? I think with... makes it a bit blurry to follow as not everything should be prefixed like that.

}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

My proposal for refactoring:

private void configureExtension(MavenSession session, Configuration configuration) {
    final Consumer<MavenProject> configureSmartTestingExtensionAction;
    if (hasModuleSpecificConfigurations(session)) {
        configureSmartTestingExtensionAction = applyModuleSpecificConfiguration(configuration);
    } else {
        final MavenProjectConfigurator mavenProjectConfigurator = new MavenProjectConfigurator(configuration);
        configureSmartTestingExtensionAction = mavenProject -> configureMavenProject(mavenProjectConfigurator, mavenProject, configuration);
    }
    session.getAllProjects().forEach(configureSmartTestingExtensionAction);
}

private boolean hasModuleSpecificConfigurations(MavenSession session) {
    final ConfigLookup lookUp = new ConfigLookup(session.getExecutionRootDirectory());
    final boolean moreThanOneConfigFile = lookUp.hasMoreThanOneConfigFile(SMART_TESTING_YML, SMART_TESTING_YAML);
    return moreThanOneConfigFile && System.getProperty(SMART_TESTING_CONFIG) == null;
}

private Consumer<MavenProject> applyModuleSpecificConfiguration(Configuration configuration) {
    return mavenProject -> {
        final ConfigLookup configLookup = new ConfigLookup(mavenProject.getBasedir().toString());
        final File dirWithConfig = configLookup.getFirstDirWithConfigOrProjectRootDir();
        final Configuration mavenProjectConfiguration = configLookup.isConfigFromProjectRootDir() ? configuration :
            ConfigurationLoader.load(dirWithConfig);
        if (mavenProjectConfiguration.isDisable()) {
            logExtensionDisableReason(logger,
                SMART_TESTING_DISABLE + " is set for module " + mavenProject.getArtifactId());
            return;
        }
        if (mavenProjectConfiguration.areStrategiesDefined()) {
            final MavenProjectConfigurator mavenProjectConfigurator =
                new MavenProjectConfigurator(mavenProjectConfiguration);
            configureMavenProject(mavenProjectConfigurator, mavenProject, mavenProjectConfiguration);
        } else {
            logger.warn(
                "Smart Testing Extension is installed but no strategies are provided for %s module. It won't influence the way how your tests are executed. "
                    + "For details on how to configure it head over to http://bit.ly/st-config",
                mavenProject.getArtifactId());
        }
    };
}

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, it took me some time to understand the code. as for assigning the consumer function to the local variable - I would use it only when the method returns some value. In this case I would use this:

    private void configureExtension(MavenSession session, Configuration configuration) {
        final File executionRootDirectory = new File(session.getExecutionRootDirectory());
        List<MavenProject> allProjects = session.getAllProjects();
        if (hasModuleSpecificConfigurations(executionRootDirectory, this::isProjectRootDirectory)) {
            allProjects.forEach(mavenProject -> applyModuleSpecificConfiguration(configuration, mavenProject));
        } else {
            final MavenProjectConfigurator mavenProjectConfigurator = new MavenProjectConfigurator(configuration);
            allProjects.forEach(mavenProject -> configureMavenProject(mavenProjectConfigurator, mavenProject, configuration));
        }
    }

    private void applyModuleSpecificConfiguration(Configuration configuration, MavenProject mavenProject) {
        final ConfigLookup configLookup = new ConfigLookup(mavenProject.getBasedir(), this::isProjectRootDirectory);
        final File dirWithConfig = configLookup.getFirstDirWithConfigOrWithStopCondition();
        final Configuration mavenProjectConfiguration = configLookup.isConfigFromProjectRootDir() ? configuration :
            ConfigurationLoader.load(dirWithConfig);
        if (mavenProjectConfiguration.isDisable()) {
            logExtensionDisableReason(logger,
                SMART_TESTING_DISABLE + " is set for module " + mavenProject.getArtifactId());
            return;
        }
        if (mavenProjectConfiguration.areStrategiesDefined()) {
            final MavenProjectConfigurator mavenProjectConfigurator =
                new MavenProjectConfigurator(mavenProjectConfiguration);
            configureMavenProject(mavenProjectConfigurator, mavenProject, mavenProjectConfiguration);
        } else {
            logger.warn(
                "Smart Testing Extension is installed but no strategies are provided for %s module. It won't influence the way how your tests are executed. "
                    + "For details on how to configure it head over to http://bit.ly/st-config",
                mavenProject.getArtifactId());
        }
    }

Just my opinion ;-)

=== Configuration File per Maven Module:
You can define configuration file per maven module. However it's not mandatory to define configuration per each module.

NOTE: Whenever you are using configuration file per maven module, make sure to define strategies & scm properties
Copy link
Member

Choose a reason for hiding this comment

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

Are scm properties really required? Why should they be?

Copy link
Contributor Author

@dipak-pawar dipak-pawar Nov 22, 2017

Choose a reason for hiding this comment

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

Yes, it needs to be define in parent config file OR define it using system properties. Because we are loading scm changes by looking into parent configuration file.

Otherwise ST couldn't find required changes between commit ranges. as it will take default range which is HEAD-HEAD~0

Copy link
Member

Choose a reason for hiding this comment

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

So it is not required but needed if you use it. Maybe we could clarify this sentence then.

* config/impl-base - parent/config/smart-testing.yml
* config/spi - parent/config/spi/smart-testing.yml
* container/api - parent/smart-testing.yml
* container/impl-base - parent/smart-testing.yml
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about having concrete file content listed and then used as an example to show effective resolution instead?

Copy link
Member

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements @dipak-pawar - two small suggestions from my side. WDYT?

properties = new HashMap<>();
}

static ConfigurationFile SmartTestingConfigurationFile() {
return new ConfigurationFile();
static ConfigurationFileBuilder SmartTestingConfigurationFile() {
Copy link
Member

Choose a reason for hiding this comment

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

Uppercase in method name is a bit unusual for Java coders unless it's a constructor. Can we rename it to simple configurationFile? Also create could be writeTo, so it could be written as

configurationFile()
    .mode("ordering")
    .applyTo("surefire")
    .inherit(Paths.get(root, SMART_TESTING_YAML).toString())
    .writeTo(Paths.get(root, SMART_TESTING_YML));

filePath = filePath.getParent().resolve(inherit);
config = getConfigParametersFromFile(filePath);
if (!config.isEmpty()) {
configs.add(0, config);
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, you can use Deque as a Stack - might make it a bit easier to understand what we really do with inheritance.

configItems.add(new ConfigurationItem("debug", SMART_TESTING_DEBUG, false));
configItems.add(new ConfigurationItem("autocorrect", SMART_TESTING_AUTOCORRECT, false));
configItems.add(new ConfigurationItem("customStrategies", SMART_TESTING_CUSTOM_STRATEGIES_PATTERN));
configItems.add(new ConfigurationItem(INHERIT, null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to register the inherit parameter? We don't use it anywhere (the getter), do we?
So do we need to have the inherit field in the class at all?

}

return count > 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would move the three methods outside of this class - they are not related to the stopCondition and are Maven-extension specific.

ConfigurationReader configurationReader = new ConfigurationReader();
final Map<String, Object> effectiveConfig = configurationReader.readConfiguration(configFile);

return ConfigurationLoader.loadAsConfiguration(effectiveConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

as you are in the same class, you can call the method directly.


private static Configuration loadEffectiveConfiguration(File configFile) {
ConfigurationReader configurationReader = new ConfigurationReader();
final Map<String, Object> effectiveConfig = configurationReader.readConfiguration(configFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

if you call this method loadEffectiveConfiguration then the method returning the final Map should have a similar name, eg: readEffectiveConfiguration - to make it transparent which part of the code handles the inheritance.

import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

class ObjectMapper {

static <T extends ConfigurationSection> T mapToObject(Class<T> aClass, Map<String, Object> map) {
<T extends ConfigurationSection> T readValue(Class<T> aClass, Map<String, Object> map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason for making it non-static? For me, this change is not necessary and the new usage doesn't bring any benefit - it is just longer - eg. see here:

-        final Configuration configuration = mapToObject(Configuration.class, yamlConfiguration);
+        final ObjectMapper objectMapper = new ObjectMapper();
+        final Configuration configuration = objectMapper.readValue(Configuration.class, yamlConfiguration);

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with it. But as @bartoszmajsak suggested me #262 (comment) & also we have a discussion about the same in mattermost.

Copy link
Member

Choose a reason for hiding this comment

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

My comment was about object mapper variable in that method, not really about the method itself being static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then I will make it static.

final Collection<TestResult> expectedTestResults = project
.applyAsLocalChanges("Adds new unit test");

expectedTestResults.addAll(project.applyAsCommits("Deletes one test", "Renames unit test"));
Copy link
Contributor

@MatousJobanek MatousJobanek Nov 28, 2017

Choose a reason for hiding this comment

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

Having this set of commits with the given configuration won't verify that the correct configuration is used.
configuration in parent dir has selecting (default) mode and new, changed strategies
for config/impl-base with the strategy new and mode selecting and you use Adds new unit test commit, which means that even if the parent configuration is read, then the test will pass - always run only the new test.
Similarly, for the junit/core module.
you can verify it removing the lines 53-56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this way we can make sure that configuration file applied correctly.

return mavenProject -> {
final ConfigLookup configLookup = new ConfigLookup(mavenProject.getBasedir(), this::isProjectRootDirectory);
final File dirWithConfig = configLookup.getFirstDirWithConfigOrWithStopCondition();
final Configuration mavenProjectConfiguration = configLookup.isConfigFromProjectRootDir() ? configuration :
Copy link
Contributor

Choose a reason for hiding this comment

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

On the previous line, you call configLookup.getFirstDirWithConfigOrWithStopCondition() and here using the method configLookup.isConfigFromProjectRootDir() you call the same method again: getFirstDirWithConfigOrWithStopCondition()
To be honest, this is very cryptic to my taste.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm asking myself, is it really worth having this complicated check there? When looking at the code, it doesn't seem to be an optimization at all - this solution is maybe more time and resource consuming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok let me see how can we make it more readable & less complicated

ConfigurationLoader.load(dirWithConfig);
if (mavenProjectConfiguration.isDisable()) {
logExtensionDisableReason(logger,
SMART_TESTING_DISABLE + " is set for module " + mavenProject.getArtifactId());
Copy link
Contributor

Choose a reason for hiding this comment

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

"Smart Testing Extension is installed but no strategies are provided for %s module. It won't influence the way how your tests are executed. "
+ "For details on how to configure it head over to http://bit.ly/st-config",
mavenProject.getArtifactId());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, it would be good to have the checks (if the ST should be installed for the module) in separated class.
As it is done for SkipInstallationChecker as well as for SkipModuleChecker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are just checking configuration.isDisable() to check to ST should be installed for module. So I don't think we should move it to separated class.

Copy link
Contributor

Choose a reason for hiding this comment

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

You check two things here:
mavenProjectConfiguration.isDisable()
and
mavenProjectConfiguration.areStrategiesDefined()

});
if (mavenProjectConfiguration.areStrategiesDefined()) {
final MavenProjectConfigurator mavenProjectConfigurator =
new MavenProjectConfigurator(mavenProjectConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

the instantiation of this class can be inside of the configureMavenProject method - you pass the configuration instance there anyway

* add configurationchecker & moduleSTInstallationChecker
return objectMapper.readValue(aClass, map);
}

<T extends ConfigurationSection> T readValue(Class<T> aClass, Map<String, Object> map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? What is the benefit of it?

Copy link
Contributor Author

@dipak-pawar dipak-pawar Dec 4, 2017

Choose a reason for hiding this comment

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

it's not necessary. But I created it to keep it clean. With mapToObject we have static factory method due to which you don't have to create ObjectMapper instance each time.

Copy link
Contributor

Choose a reason for hiding this comment

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

With mapToObject we have static factory method due to which you don't have to create ObjectMapper instance each time.

if it is a static method, then you don't create any instance of the class.
This approach is wrongly used in several cases in the ST code - instead of using classes such as ConfigurationChecker and ConfigurationReader where you need to create an instance of them and then call one method (without any need to keep the state) you can use them as static "util" classes.

for (String key: inner.keySet()) {
if (!Map.class.isAssignableFrom(inner.get(key).getClass()) || !effective.containsKey(key)){
effective.put(key, inner.get(key));
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

my opinion: instead of continue I would use the whole if-else statement.


private Map<String, Object> overwriteInnerProperties(Map<String, Object> effective, Map<String, Object> inner) {
for (String key: inner.keySet()) {
if (!Map.class.isAssignableFrom(inner.get(key).getClass()) || !effective.containsKey(key)){
Copy link
Contributor

@MatousJobanek MatousJobanek Dec 4, 2017

Choose a reason for hiding this comment

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

is it possible to move this check to some separated method with a name expressing what the if does?
something like: isTrivialPropertyOrNotContainedInMap
or maybe do it the other way around - isNonTrivialPropertyContainedInMap


Map<String, Object> effectiveConfig = configs.pollFirst();
while (!configs.isEmpty()) {
effectiveConfig = overwriteInnerProperties(effectiveConfig, configs.pollFirst());
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to assign the returned value to the variable - the instance of effectiveConfig is modified directly.

public final TestBed testBed = new TestBed(GIT_CLONE);

@Test
public void should_load_configuration_from_modules_config_file_for_local_changes_where_parent_config_file_has_only_strategies_defined() {
Copy link
Contributor

Choose a reason for hiding this comment

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

my opinion: can be the name a bit shorter? eg:
should_load_config_from_module_config_file_for_local_changes_instead_of_parent_one
similarly for the following one.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, what is the difference between these two tests? Both of them verify if the module related config file is taken, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there is difference, in one test - parent config having only strategy defined. & in another parent config having scm & strategy defined which is already mentioned in method name

Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to merge it into one test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I will merge it into one.

If config file doesn't contain `inherit` parameter. Then Smart Testing won't inherit any configuration properties.

=== Configuration File per Maven Module:
You can define configuration file per maven module. However it's not mandatory to define configuration per each module.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not necessary I think - it describes the same as on the line 39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's good to have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Smart Testing is looking in each module's root dir for configuration file. If it didn't find config file there, it'll look recursively for the first config file in the parent dir containing pom.xml.

has the very same meaning as

You can define configuration file per maven module. However, it's not mandatory to define configuration per each module.

both of them says that you can define the config file in maven the module (or in parent dirs) and if it's not there (which means that it is not mandatory) it will look recursively for the first config file

You can define configuration file per maven module. However it's not mandatory to define configuration per each module.

NOTE: Whenever you are using configuration file per maven module, make sure to define strategies in project's
parent configuration file or using system property. If you are running tests against range of commits, you need to define scm properties in project's parent config file or using system property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever you are using configuration file per maven module, make sure to define strategies in project's parent configuration file or using system property

This is very confusing - it can be specified also inside of the module config file. I would remove it.

Copy link
Contributor Author

@dipak-pawar dipak-pawar Dec 5, 2017

Choose a reason for hiding this comment

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

no you can't specify it inside module config file. You have to define it in project's parent config file or using system property. Otherwise Smart testing will load parent configuration having no strategy defined which will disable ST having reason no strategy defined at least in case of mvn extension

This is the same case of scm properties as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Then describe it in this way as you just did in your comment.

This also brings a good point - that you can use at these places your ModuleSTInstallationChecker logic

- smart-testing.yml #<!--3-->
```

<1>
Copy link
Contributor

Choose a reason for hiding this comment

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

these numbers are not correctly formatted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean numbers mentioned in xml config file

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the numbers <1> in the resulting documentation page

----

<2>
parent/config/spi/smart-testing.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

in the other cases, there is a dot at the beginning. not here. is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I missed it here. thanks for catching it up.

@@ -43,6 +108,9 @@ NOTE: You can use either a `.yml` or `.yaml` extension for this file.
|===
|Field | Description

a| inherit
a| This is used to define absolute or relative path for configuration file from where you want to overwrite paremeters not defined in current config file.
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't overwrite parameters that are not defined - there is nothing to overwrite ;-)
I think that enough is saying:
This is used to define absolute or relative path to parent configuration file


NOTE: Whenever you are using configuration file per maven module, make sure to define strategies in project's
parent configuration file or using system property. If you are running tests against range of commits, you need to define scm properties in project's parent config file or using system property.
It won't work if you define scm properties in any module's config file and not in parent config file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, create an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #274

@dipak-pawar
Copy link
Contributor Author

@bartoszmajsak , @MatousJobanek Can you have a look into it?

Copy link
Contributor

@MatousJobanek MatousJobanek left a comment

Choose a reason for hiding this comment

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

Just one small thing, otherwise, I'm good with it. Good job @dipak-pawar


import static org.arquillian.smart.testing.configuration.Configuration.SMART_TESTING_DISABLE;

public class SkipSTInstallationChecker {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class name is very similar to: SkipInstallationChecker it's very confusing.
Please, merge these two classes. The constructor can stay same, you just add another method shouldSkipForConfig(Configuration config) which you will call at the places where you call your SkipSTInstallationChecker

@bartoszmajsak
Copy link
Member

bartoszmajsak commented Dec 12, 2017

@dipak-pawar thanks for your work! Please take care of the merge conflict and merge. Just clean up extended commit message as we don't need a log of all intermediate commits :)

@bartoszmajsak bartoszmajsak changed the title feat(#114): Implement configuration file per module. feat(#114): implements configuration file per module. Dec 12, 2017
@dipak-pawar dipak-pawar merged commit a2f72dc into arquillian:master Dec 13, 2017
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.

3 participants