Summary view navigation #150

JKutscha · 2021-07-16T16:28:55Z

Issue #62

Added navigation via Buttons in Summary view

* home: get back to index.html in basedir * <: back one page * \>: forward one page

JKutscha · 2021-07-16T19:07:06Z

This should all run fine. I run all tests with my eclipse on Windows and Linux successfully, but for whatever reason on Windows mvn clean verify fails. Can you help me and tell me how this can be?

LorenzoBettini · 2021-07-17T09:47:01Z

You removed the PitNotifier and this breaks the synchronization I guess and makes the tests unreliable.

Also removed checks in PitclipseUiRunnerTest for coverage generatetd, because there is no report generatetd anymore

JKutscha · 2021-07-18T08:02:53Z

You removed the PitNotifier and this breaks the synchronization I guess and makes the tests unreliable.

@LorenzoBettini now they are reliable again in regard of waiting for the result.

JKutscha · 2021-07-18T08:23:34Z

@LorenzoBettini I ran the test now multiple times on my Windows PC and they are always successful. Would you mind taking a look?

LorenzoBettini · 2021-07-18T15:53:17Z

You removed the PitNotifier.java and you removed a few checks in the existing tests...
Please don't change the architecture of the existing testing infrastructure...
if you can't succeed in testing the new scenarios I'll try to write the tests myself on top of your new feature, but please first restore the existing working tests

JKutscha · 2021-07-18T16:14:12Z

I only removed the part of the test, where empty results are expected and no html is created and the checks were to look at the html.

Furthermore all checks which were present are working fine and only my test case is failing, but only online. Tested them on windows and linux multiple times.

tests/org.pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/swtbot/PitNotifier.java

...s/org.pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/PitclipseUiRunnerTest.java

LorenzoBettini · 2021-07-18T16:28:44Z

I only removed the part of the test, where empty results are expected and no html is created and the checks were to look at the html.

Furthermore all checks which were present are working fine and only my test case is failing, but only online. Tested them on windows and linux multiple times.

See my detailed comments

LorenzoBettini · 2021-07-18T17:09:57Z

Furthermore all checks which were present are working fine and only my test case is failing, but only online. Tested them on windows and linux multiple times.

but did you also run the Maven build?

The failure you get in the CI is as follows:

Failures: 
  PitclipsePitSummaryViewTest.navigateWithButtons:60 
Expected: a string ending with "index.html"
     but: was "file:///D:/a/pitclipse/pitclipse/tests/org.pitest.pitclipse.ui.tests/target/work/data/.metadata/.plugins/org.pitest.pitclipse.core/html_results/202107180752/foo.bar/Foo.java.html"

JKutscha · 2021-07-18T17:11:29Z

Yes, I also ran maven.

JKutscha · 2021-07-18T18:29:14Z

Yes, I also ran maven.

LorenzoBettini · 2021-07-20T07:43:57Z

Usually when it works locally but not on the CI you have synchronization problems, maybe because the CI virtual environment is faster than your machine or vice-versa. Note that now you have a failure in the macOS CI environment, while before you had it in the Windows virtual environment.

Once again, my suggestion is to keep the implementation extremely simple (e.g., don't keep track of the url string, just set it in the browser and let the buttons do the rest); similarly, tests must also be simple and reproducible. For example, don't rely on the browser to be reactive on changing pages. If I remember correctly there are lots of asynchronous operations going on there. Moreover, rely on the summary view automatically opened after running PIT, and don't try to open the view yourself: the browser might be out of date.

Once again, if you want I can simplify both the implementation and try to implement the tests.

JKutscha · 2021-07-20T08:43:31Z

Usually when it works locally but not on the CI you have synchronization problems, maybe because the CI virtual environment is faster than your machine or vice-versa. Note that now you have a failure in the macOS CI environment, while before you had it in the Windows virtual environment.

Once again, my suggestion is to keep the implementation extremely simple (e.g., don't keep track of the url string, just set it in the browser and let the buttons do the rest); similarly, tests must also be simple and reproducible. For example, don't rely on the browser to be reactive on changing pages. If I remember correctly there are lots of asynchronous operations going on there. Moreover, rely on the summary view automatically opened after running PIT, and don't try to open the view yourself: the browser might be out of date.

Once again, if you want I can simplify both the implementation and try to implement the tests.

What do you mean by don't keep track of the url string? Do you refer to the "Home" button?

How would you test the navigation with the buttons, if you dont wait for the pages to load?

The summary view itself gets disposed and the browser can't be out of date, because a new one gets created while opening.

If you want to change things feel free, but I want to know why things are changed. So I can learn from that for the future.

JKutscha · 2021-07-22T14:56:52Z

It seems the issue we were having with macOS were causing the reliability issues I was having.

JKutscha · 2021-07-23T06:37:03Z

It seems the issue we were having with macOS were causing the reliability issues I was having.

Or not ...

@LorenzoBettini Is there any way to know which version of Eclipse and which Browser is used on the server site?

bundles/org.pitest.pitclipse.ui/src/org/pitest/pitclipse/ui/view/PitView.java

...st.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/behaviours/pageobjects/PitSummaryView.java

tests/org.pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/swtbot/PitResultNotifier.java

tests/org.pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/swtbot/ResultsParser.java

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/AbstractPitclipseSWTBotTest.java

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/PitclipsePitSummaryViewTest.java

echebbi · 2021-07-24T04:45:34Z

Is there any way to know which version of Eclipse and which Browser is used on the server site?

@JKutscha are you talking about the CI server? It should use the versions defined in the target plaform. Those should be the same as when you run Pitclipse from your IDE if your configured it with the TP. And regarding the browser, according to the documentation SWT embeds Safari on macOS.

Tests are still not working, because the bot waits not for page loads?!

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/PitclipsePitSummaryViewTest.java

JKutscha · 2021-07-29T07:08:16Z

IIRC until now the tests were waiting for the Mutations view to send an event telling it received the results from Pit. Your mechanism waits for mutations results to be available, assumes that the view is updated then immediately moves on. I want to make sure I understand correctly and that there's no synchronization issues (e.g., when the view has a lot of mutations to display).

It waited for the PitSummary (PitView) to have the HTML set and parses this HTML source.

* own wait condition in PitResultNotifier * actions now have the same suffix and are no longer fields, was not necessary

…aryView)

LorenzoBettini · 2021-07-30T13:44:47Z

@JKutscha @echebbi I've just created a companion PR #169 because I'd like the code to be analyzed by SonarCloud (which cannot currently be done for external PRs, see #161). I basically took the branch of @JKutscha and pushed to this repo (using the same branch name) and created a separate PR. I'd just like to experiment with such a mechanism...

LorenzoBettini

As you see in the comments, I guess I should not review the tests since when #167 is merged this PR should be updated as well, is that right?

Besides that, I have a few concerns about uncovered lines (see the comments), see also https://sonarcloud.io/component_measures?id=org.pitest%3Aorg.pitest.pitclipse&metric=new_coverage&pullRequest=169&selected=org.pitest%3Aorg.pitest.pitclipse%3Abundles%2Forg.pitest.pitclipse.ui%2Fsrc%2Forg%2Fpitest%2Fpitclipse%2Fui%2Fview%2FPitView.java&view=list

Thank you!

bundles/org.pitest.pitclipse.ui/src/org/pitest/pitclipse/ui/view/PitView.java

...st.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/behaviours/pageobjects/PitSummaryView.java

.../org.pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/behaviours/pageobjects/Views.java

...g.pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/behaviours/steps/PitclipseSteps.java

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/AbstractPitclipseSWTBotTest.java

…tion

JKutscha · 2021-08-03T21:17:27Z

@LorenzoBettini @echebbi please review this PR, if you find some time. From my part, this PR is ready.

LorenzoBettini

I'm basically fine, but I left a few (cosmetic) comments, some questions, and one crucial change to be done.

bundles/org.pitest.pitclipse.ui/src/org/pitest/pitclipse/ui/view/PitView.java

LorenzoBettini · 2021-08-04T09:43:22Z

bundles/org.pitest.pitclipse.ui/src/org/pitest/pitclipse/ui/view/PitView.java

+    /**
+     * Set URL to blank and reset homeUrl, to have a clean browser after
+     * creation<br>
+     * <b>Needed</b> for testing purposes


This is not needed anymore by tests, isn't it?
Indeed it's private

This is still needed.
It is called while creating and if an update() has no file.

Directly from the tests? By reflection?

No, in the methods update(...) and createPartControl(...).

bundles/org.pitest.pitclipse.ui/src/org/pitest/pitclipse/ui/view/PitView.java

...st.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/behaviours/pageobjects/PitSummaryView.java

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/AbstractPitclipseSWTBotTest.java

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/PitclipsePitSummaryViewTest.java

JKutscha · 2021-08-04T13:03:51Z

Mostly ask for cosmetic changes. I will also take the time to review the code from the IDE. I mostly want to check the way you changed the synchronization mechanism.

@echebbi Are you happy with the result and can we merge from your POV?

LorenzoBettini

I'm fine with that... I only have a doubt on that Javadoc on the private method, but probably it's just my misunderstanding (when I read "Needed for tests" I mean a method, typically package-private, that is meant to be called directly by tests...)

@echebbi , if you're fine with the PR please merge it

LorenzoBettini · 2021-08-06T09:32:39Z

@echebbi can I merge this or do you want to further review it?

echebbi · 2021-08-06T09:44:20Z

@echebbi can I merge this or do you want to further review it?

@LorenzoBettini I don't have the time to review it right now but @JKutscha addressed my previous comments so if it looks good to you please proceed

JKutscha and others added 3 commits July 16, 2021 12:57

added navigation buttons:

ddc8806

* home: get back to index.html in basedir * <: back one page * \>: forward one page

Restructered some tests and added test for summary

aee9697

Fix for test of PitclipsePitSummaryViewTest

d71a67e

JKutscha added 2 commits July 18, 2021 09:35

Added wait condition to PitResultNotifier

7e9894f

Also removed checks in PitclipseUiRunnerTest for coverage generatetd, because there is no report generatetd anymore

Changed timeouts to fit better

54a5cd0

LorenzoBettini requested changes Jul 18, 2021

View reviewed changes

Unused class which I forgot to delete

c6f83e6

Merge branch 'master' into Summary_View_Navigation

bc44bc9

JKutscha requested a review from LorenzoBettini July 22, 2021 14:56

Merge branch 'master' into Summary_View_Navigation

0219999

echebbi requested changes Jul 24, 2021

View reviewed changes

Merge branch 'master' into Summary_View_Navigation

8569610

JKutscha marked this pull request as draft July 24, 2021 09:46

Removed bot click events and use reference

e3594ed

Tests are still not working, because the bot waits not for page loads?!

JKutscha commented Jul 24, 2021

View reviewed changes

...pitest.pitclipse.ui.tests/src/org/pitest/pitclipse/ui/tests/PitclipsePitSummaryViewTest.java Outdated Show resolved Hide resolved

JKutscha added 4 commits July 29, 2021 09:28

Code review fixes for comments of echebbi

65f5216

* own wait condition in PitResultNotifier * actions now have the same suffix and are no longer fields, was not necessary

Wait for browser to load page in test instead of constucutor (PitSumm…

48b4a75

…aryView)

Added own test for testing while empty view

07892bb

Removed not used fail

96252b1

JKutscha mentioned this pull request Jul 30, 2021

Restructered tests to not rely on the Summary View #167

Merged

LorenzoBettini mentioned this pull request Jul 30, 2021

Summary view navigation #169

Merged

LorenzoBettini requested changes Jul 30, 2021

View reviewed changes

JKutscha added 2 commits August 2, 2021 09:05

Merge branch 'master' into Summary_View_Navigation

ca75d8f

Close view using bot | create buttons and add them

d3ee974

LorenzoBettini mentioned this pull request Aug 2, 2021

UI utils #170

Merged

Jonas Kutscha and others added 3 commits August 3, 2021 14:39

Merge remote-tracking branch 'origin/master' into Summary_View_Naviga…

615e174

…tion

Removed double brwoser creation, merge oversight

2467ab1

Test fix for codeCoverageReport generated after merge

6e6f709

JKutscha requested review from LorenzoBettini and echebbi August 3, 2021 21:17

LorenzoBettini requested changes Aug 4, 2021

View reviewed changes

Jonas Kutscha and others added 2 commits August 4, 2021 13:54

Review fixes

2421be6

Merge branch 'master' into Summary_View_Navigation

fd53cc8

JKutscha requested a review from LorenzoBettini August 4, 2021 13:02

LorenzoBettini approved these changes Aug 5, 2021

View reviewed changes

echebbi approved these changes Aug 6, 2021

View reviewed changes

echebbi merged commit 157167f into pitest:master Aug 7, 2021

echebbi mentioned this pull request Aug 7, 2021

Can only navigate forward in the coverage report window #62

Closed

JKutscha deleted the Summary_View_Navigation branch August 9, 2021 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Summary view navigation #150

Summary view navigation #150

JKutscha commented Jul 16, 2021 •

edited

Loading

JKutscha commented Jul 16, 2021

LorenzoBettini commented Jul 17, 2021

JKutscha commented Jul 18, 2021

JKutscha commented Jul 18, 2021

LorenzoBettini commented Jul 18, 2021

JKutscha commented Jul 18, 2021

LorenzoBettini commented Jul 18, 2021

LorenzoBettini commented Jul 18, 2021

JKutscha commented Jul 18, 2021

JKutscha commented Jul 18, 2021

LorenzoBettini commented Jul 20, 2021

JKutscha commented Jul 20, 2021

JKutscha commented Jul 22, 2021

JKutscha commented Jul 23, 2021

echebbi commented Jul 24, 2021

JKutscha commented Jul 29, 2021

LorenzoBettini commented Jul 30, 2021

LorenzoBettini left a comment

JKutscha commented Aug 3, 2021

LorenzoBettini left a comment

LorenzoBettini Aug 4, 2021

JKutscha Aug 4, 2021

LorenzoBettini Aug 5, 2021

JKutscha Aug 5, 2021

JKutscha commented Aug 4, 2021

LorenzoBettini left a comment

LorenzoBettini commented Aug 6, 2021

echebbi commented Aug 6, 2021

Summary view navigation #150

Summary view navigation #150

Conversation

JKutscha commented Jul 16, 2021 • edited Loading

JKutscha commented Jul 16, 2021

LorenzoBettini commented Jul 17, 2021

JKutscha commented Jul 18, 2021

JKutscha commented Jul 18, 2021

LorenzoBettini commented Jul 18, 2021

JKutscha commented Jul 18, 2021

LorenzoBettini commented Jul 18, 2021

LorenzoBettini commented Jul 18, 2021

JKutscha commented Jul 18, 2021

JKutscha commented Jul 18, 2021

LorenzoBettini commented Jul 20, 2021

JKutscha commented Jul 20, 2021

JKutscha commented Jul 22, 2021

JKutscha commented Jul 23, 2021

echebbi commented Jul 24, 2021

JKutscha commented Jul 29, 2021

LorenzoBettini commented Jul 30, 2021

LorenzoBettini left a comment

Choose a reason for hiding this comment

JKutscha commented Aug 3, 2021

LorenzoBettini left a comment

Choose a reason for hiding this comment

LorenzoBettini Aug 4, 2021

Choose a reason for hiding this comment

JKutscha Aug 4, 2021

Choose a reason for hiding this comment

LorenzoBettini Aug 5, 2021

Choose a reason for hiding this comment

JKutscha Aug 5, 2021

Choose a reason for hiding this comment

JKutscha commented Aug 4, 2021

LorenzoBettini left a comment

Choose a reason for hiding this comment

LorenzoBettini commented Aug 6, 2021

echebbi commented Aug 6, 2021

JKutscha commented Jul 16, 2021 •

edited

Loading