Mutator configuration

[JKutscha]

Issue #114

[LorenzoBettini]

Besides a few questions and small changes, I left a few hints that in my opinion might be the cause of flaky tests

[JKutscha]

@LorenzoBettini I found the reason why the tests were failing randomly, it was a problem with how we were getting the run menu and we relied on the bot to have the correct shell, but it was not always the correct one. This is now fixed in #175.
[...]
Please merge it to your branch

Your solution fixes this problem, but #175 fixes another big flaw. Please consider merging this with master.

The reason why I would like to get this merged first, is that on my red hat pc all tests tend to fail now sometimes and this is regarded to the fixes which are done there.

I also got to the point of waiting for the shell to be active, but this alone does not always fix the problem for me.

[LorenzoBettini]

I'm ok with the current state of this PR; we'll just wait a bit to see if @LorenzoBettini has the time to make some comments.

Looks like we're working (and probably found a fix) for flaky UI tests...

I'd also like to have full coverage on the code of this PR and it looks like I have to do that myself on my branch

[echebbi]

Looks like we're working (and probably found a fix) for flaky UI tests...

That's what I understood from the comments; thanks for your insights about why KDE might have brought the bugs to show up.

I'd also like to have full coverage on the code of this PR and it looks like I have to do that myself on my branch

You sound a bit annoyed, I'm sorry if you feel like the only one who cares about code coverage. Do you have any update on #161? It would help monitoring code coverage without having to keep another branch synchronized and to go back and forth between two PRs.

[LorenzoBettini]

@JKutscha I have a few problems following GitHub notifications emails... did you merge in this branch the other PRs we're still discussing upon? (#177 and #175)

[JKutscha]

@JKutscha I have a few problems following GitHub notifications emails... did you merge in this branch the other PRs we're still discussing upon? (#177 and #175)

I miss merged some other stuff of mine into this branch by accident, which I directly reverted.

[LorenzoBettini]

@JKutscha it looks like the fix I proposed highly reduced the failure of those UI tests, but not completely. I pushed another commit that hopefully fixes the problem for good, f85742b

Note that this really forces the activation of the Eclipse workbench, because when the test fails it's because the current shell is not active at all. That's why, also in #175 (comment), I was suggesting to force the activation of the workbench programmatically like I've just done.

[JKutscha]

@JKutscha it looks like the fix I proposed highly reduced the failure of those UI tests, but not completely. I pushed another commit that hopefully fixes the problem for good, f85742b

Note that this really forces the activation of the Eclipse workbench, because when the test fails it's because the current shell is not active at all. That's why, also in #175 (comment), I was suggesting to force the activation of the workbench programmatically like I've just done.

The same was happening to me and not just in the UI tests for this newly created test cases. This was the reason why I created #175 and don't want to rely on the focus at all while getting the menu.

[LorenzoBettini]

@JKutscha it looks like the fix I proposed highly reduced the failure of those UI tests, but not completely. I pushed another commit that hopefully fixes the problem for good, f85742b
Note that this really forces the activation of the Eclipse workbench, because when the test fails it's because the current shell is not active at all. That's why, also in #175 (comment), I was suggesting to force the activation of the workbench programmatically like I've just done.

The same was happening to me and not just in the UI tests for this newly created test cases. This was the reason why I created #175 and don't want to rely on the focus at all while getting the menu.

So you mean that code like the following one

        SWTBotMenuHelper menuHelper = new SWTBotMenuHelper();
        menuHelper.findMenu(...);

does not need an active shell at all?

[JKutscha]

[...]
So you mean that code like the following one

        SWTBotMenuHelper menuHelper = new SWTBotMenuHelper();
        menuHelper.findMenu(...);

does not need an active shell at all?

Yours would, but not this one:

        SWTBotMenuHelper menuHelper = new SWTBotMenuHelper();
        menuHelper.findWorkbenchMenu(...);

It would still require the workbench to be opened, but would not need to shift the focus.

[LorenzoBettini]

Interesting! So should we first merge #175 and then try to apply the same approach also here?
For #175 there's still the timeout stuff to be fixed.

[JKutscha]

Interesting! So should we first merge #175 and then try to apply the same approach also here?

This was my intent.

For #175 there's still the timeout stuff to be fixed.

I was waiting on a message from @echebbi on that or a final call from your site.

[LorenzoBettini]

Interesting! So should we first merge #175 and then try to apply the same approach also here?

This was my intent.

For #175 there's still the timeout stuff to be fixed.

I was waiting on a message from @echebbi on that or a final call from your site.

I don't want to speak for @echebbi but your solution introduced a fixed hardcoded timeout, which in general is to avoid. I'd remove that mechanism and then if @echebbi wanted it back we could re-introduce that, but I'm pretty sure that's unlikely ;)

[LorenzoBettini]

@JKutscha in my branch the part that needs to have full coverage is this one:

    private class UpdateDialogOnCurrentMutatorGroupChanged extends SelectionAdapter {
        @Override
        public void widgetSelected(SelectionEvent event) {
            final String old = currentMutatorGroup;
            currentMutatorGroup = (String) event.widget.getData();
            if (((Button) event.widget).getSelection() && !old.equals(currentMutatorGroup)) {
                updateLaunchConfigurationDialog();
                disableTableIfUnused();
            }
        }
    }

The if is marked as 1 of 4 branches missed, and I'm not able to understand what is still to be tested...

[JKutscha]

@JKutscha in my branch the part that needs to have full coverage is this one:

    private class UpdateDialogOnCurrentMutatorGroupChanged extends SelectionAdapter {
        @Override
        public void widgetSelected(SelectionEvent event) {
            final String old = currentMutatorGroup;
            currentMutatorGroup = (String) event.widget.getData();
            if (((Button) event.widget).getSelection() && !old.equals(currentMutatorGroup)) {
                updateLaunchConfigurationDialog();
                disableTableIfUnused();
            }
        }
    }

The if is marked as 1 of 4 branches missed, and I'm not able to understand what is still to be tested...

I will take a look right now.

[JKutscha]

The if is marked as 1 of 4 branches missed, and I'm not able to understand what is still to be tested...

I don't know why this is shown as one branch missing, because if I run the tests locally with EclEmma I get this:

But I must admit, looking at the tests, I'm not sure if the tests regarding this particular behavior are good at all.

   @Test
    public void pressMutatorGroupButtons() { // NOSONAR
        PAGES.getRunMenu().setMutatorGroup(TEST_CONFIG_NAME, Mutators.OLD_DEFAULTS);
        PAGES.getRunMenu().setMutatorGroup(TEST_CONFIG_NAME, Mutators.DEFAULTS);
        PAGES.getRunMenu().setMutatorGroup(TEST_CONFIG_NAME, Mutators.STRONGER);
        PAGES.getRunMenu().setMutatorGroup(TEST_CONFIG_NAME, Mutators.ALL);
        PAGES.getRunMenu().setMutatorGroup(TEST_CONFIG_NAME, Mutators.DEFAULTS);
    }

I wrote this one, but I don't think it does anything useful and I'm not sure how to make it useful.

[LorenzoBettini]

That's not my case:

this could mean that in your case is covered because of some kind of timeout that does not take place?

[LorenzoBettini]

@JKutscha on my branch I've already merged with master after #175 has been merged into master. I solved the conflict, please check that I did it correctly.

We still have to understand why getSelection() is not covered... are you sure that condition is really useful?

[LorenzoBettini]

@JKutscha in this code

    private class UpdateDialogOnCurrentMutatorGroupChanged extends SelectionAdapter {
        @Override
        public void widgetSelected(SelectionEvent event) {
            final String old = currentMutatorGroup;
            currentMutatorGroup = (String) event.widget.getData();
            if (((Button) event.widget).getSelection() && !old.equals(currentMutatorGroup)) {
                updateLaunchConfigurationDialog();
                disableTableIfUnused();
            }
        }
    }

I don't see how old and currentMutatorGroup could be different without the radio button being selected. So, if I understand correctly the code above, removing ((Button) event.widget).getSelection() makes the code equivalent. I pushed that on my branch...

[JKutscha]

@JKutscha on my branch I've already merged with master after #175 has been merged into master. I solved the conflict, please check that I did it correctly.

This looks good to me on your branch.

We still have to understand why getSelection() is not covered... are you sure that condition is really useful?

I looked into your code and if everything works as expected I think your condition will work just fine.

[JKutscha]

@LorenzoBettini Should I cherry-pick your commits or how should we continue this PR?

[LorenzoBettini]

If you want to further work on this branch you should merge yours with mine. Otherwise, if you agree with the current version of my branch, in particular if you agree that the additional check of the selection is useless, I can simply go on merging my PR.

[JKutscha]

If you want to further work on this branch you should merge yours with mine. Otherwise, if you agree with the current version of my branch, in particular if you agree that the additional check of the selection is useless, I can simply go on merging my PR.

I‘m fine, if you merge your PR.

[LorenzoBettini]

@JKutscha @echebbi the companion PR #174 has been merged!
Thanks a lot!