Don't skip tests when test.selenium=true

[brideck]

Because of the changes to ITBase for Selenium support, a dozen or so tests were inadvertently being skipped when the test.selenium flag is set to true. This updates the affected test classes to override the inherited RunWith setting.

[werpu]

Hi I don´t think this is a good idea, then the tests always will be performed no matter whether the selenium tests are enabled or not, this is a viable option but then you run those tests always and disable the environment variable switch entirely.

Following code is responsible: SeleniumArquilianRunner: @Override protected boolean isIgnored(FrameworkMethod child) { if(!"true".equals(System.getProperty("test.selenium"))) { return true; } return super.isIgnored(child); }

For whatever reason the super.isIgnored returns false for those tests (the fix might be simply to return a full true here instead of relying on super, I have to check what Arquilian does in this call)

[brideck]

Hi I don´t think this is a good idea, then the tests always will be performed no matter whether the selenium tests are enabled or not

Isn't that what we want for these tests? If someone writes a selenium alternative for them, then we can switch this back so it will run the selenium version and skip this one. But for cases like this where there is only one implementation, we would absolutely want them to run regardless of what test.selenium is set to. Otherwise, you're making me run the TCK twice in order to actually run all of the tests.

[brideck]

Following code is responsible:

SeleniumArquilianRunner:

@OverRide protected boolean isIgnored(FrameworkMethod child) {
    if(!"true".equals(System.getProperty("test.selenium"))) {
        return true;
    }
    return super.isIgnored(child);
}

For whatever reason the super.isIgnored returns false for those tests (the fix might be simply to return a full true here instead of relying on super, I have to check what Arquilian does in this call)

Also, the code you identified as responsible is not how the code reads in Git. It has:

    @Override
    protected boolean isIgnored(FrameworkMethod child) {
        if("true".equals(System.getProperty("test.selenium"))) {
            return true;
        }
        return super.isIgnored(child);
    }

Note the lack of '!', which means that any execution where test.selenium is set to true will skip any test that hits this code, if I'm reading it correctly. The super doesn't even come into play. I don't think having the '!' helps either for what it's worth, as that means any execution where test.selenium is set to false will skip these tests.

[werpu]

Ok sorry, I was yesterday still a little bit fever dizzy. I reread the issue and got it. I revoke my "veto" your patch is absolutely fine. Sorry for having this delayed.

The switch is the issue here and yes the fix is fine, go ahead and merge it.

As for the explanation of the above, that switch basically turns off the standard tests if you set -Dtest.selenium=true unfortunately I caused a sideffect by this which were unintentional! The idea behind it was that you want to run the selenium tests as special test run, and so that both do not get intermixed.

So by all means please go ahead and merge, and thanks for jumping in here!