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

new diff view #1636

Open
feckertson opened this issue Jan 14, 2022 · 10 comments
Open

new diff view #1636

feckertson opened this issue Jan 14, 2022 · 10 comments

Comments

@feckertson
Copy link

feckertson commented Jan 14, 2022

I am not really a fan of the underlines and squiggly underlines for the new diff view rather than having light background colors to signify the status of mutants. They really clutter up the display.

Can there be a way to configure this?

... or maybe making the different choice of underlining the unusual thing (the diff) and continuing to use background highlighting for the normal thing (the original code plus its mutation status).

@nicojs
Copy link
Member

nicojs commented Jan 15, 2022

You're not the first one with this feedback, and I kind of agree.

Feel free to make a mockup of the diff view in combination with surviving and killed mutants.

@feckertson
Copy link
Author

The new diff display when an indicator is "activated" is certainly superior to the old way of striking out . Kudos for that.

Personally, I would be content with the new view if it simply did not bother trying to indicate which exact portions of a line have a problem, i.e., if the underlines were not displayed, just the indicators.

image

I realize that a minor loss of fidelity is created by moving the indicators to the ends of the lines and it seems like the underlines were intended to make up for that loss. Unfortunately some easily overlooked clutter (embedded indicators) has been replaced with a different kind of clutter that is impossible to overlook.

I doubt there is any way to retain all of the lost information, especially when mutations overlap, but a potential option for retaining some of the fidelity while still moving the indicators to the ends of the lines is to use different shapes for different mutation categories. This approach would be my preference, especially if the shape-to-category connection is easy to grap (using block letters, perhaps).

image

The original placement of the indicators was not really a problem for me. The main difficulty was that the indicators got lost in a sea of background highlighting which did not provide additional value. Applying the new diff display in conjunction with the old indicator layout but without any code highlighting would also be a reasonable solution (along with the current banner bar enhancements, of course).

image

For me, the highlighting (new or old) doesn't really convey anything that the indicators don't convey on their own. If nothing else, the ability to disable the highlighting would be a plus. I asked my non-technical wife for her opinion of the display (new and old) and her response for both was "too busy", "trying to do too much".

The remainder of my comments are slightly off topic, but they stem from a closer inspection of the display while working on this reply....

  • The new indicators do not show the mutation ids. It might be nice if the ids showed up in their hovers.
  • One thing the code highlighting does provide is a visual sense of the mutation score when viewing an individual file. The same can be gleaned from the counts in the banner, but not without mental exercise. Representing it in a "pie chart" in the banner could be a way to provide the same information when the code is not highlighted (including the numbers might not work in general).
    image
  • The icon for the Killed button is misleading. It looks actionable. Maybe something like a thumbs up icon would be better.
  • Clicking on a line of code cycles through the mutation indicators on that line of code, but it cycles through them backwards.

@commonquail
Copy link

commonquail commented Feb 3, 2022

The on-click behaviour to toggle the display of mutants with nested mutants like in

image

is a complete mess. The browser jumps all over the place to focus on the activated mutant, which eventually turns into the surrounding block removal mutation (under foreach) and scrolls the entire block outside the browser window.

  • Why does clicking one set of mutations activate a different set of mutations?
  • Why does the click target move from under the cursor (i.e. why does the browser scroll)?

Because the diff view inserts new lines it also displaces later mutation toggles. When a later toggle is activated the still active, earlier mutation disappears, restoring the original position of the later toggle. This also moves the click target away from under the cursor, all without moving the browser window.

The diff renderer can be really janky:

image

image

@nicojs
Copy link
Member

nicojs commented Feb 3, 2022

The new indicators do not show the mutation ids. It might be nice if the ids showed up in their hovers.

I disagree. Mutant id's can be any string. Long names, GUIDs, etc. I'm planning to use longer IDs in the future. The ID should be a technical ID, not really something to communicate to end users.

Personally, I would be content with the new view if it simply did not bother trying to indicate which exact portions of a line have a problem, i.e., if the underlines were not displayed, just the indicators.

This is heavily debated in #1578. The consensus seems to be that the underline is not clear enough.

The original placement of the indicators was not really a problem for me.

It wasn't for me either. The problem became apparent when I implemented the diff view. The mutated line doesn't include the mutant indicators and thus is not a true "diff".

I am now thinking to simply make the mutant indicators bigger. Would that help you?

Why does clicking one set of mutations activate a different set of mutations?

I don't exactly understand this question. Clicking mutants cycles through the mutants that are active on the position of the click, so not only the mutants starting on that line. It does so from "smallest" to "largest" in scope. I thought that it would be clear, but apparently, it isn't for you? What is your expectation here?

@feckertson is this what you mean with "cycles through them backwards"?

Why does the click target move from under the cursor (i.e. why does the browser scroll)?

I implemented it for the next-previous buttons on the top left and then just let that behavior also work for clicking mutants. It only does that when the mutant isn't already in the viewport so I didn't run into the browser "jumping all over the place". I think we could change the behavior to only scroll when using the next-previous buttons.

Because the diff view inserts new lines it also displaces later mutation toggles. When a later toggle is activated the still active, earlier mutation disappears, restoring the original position of the later toggle. This also moves the click target away from under the cursor, all without moving the browser window.

Yes, the code jumps around a bit, but there is not a lot we can do about that. I also think it isn't that big of a deal. Example:

html-report

Do you see a problem that I don't?

@commonquail
Copy link

commonquail commented Feb 3, 2022

Clicking mutants cycles through the mutants that are active on the position of the click, so not only the mutants starting on that line. It does so from "smallest" to "largest" in scope. I thought that it would be clear, but apparently, it isn't for you? What is your expectation here?

It's clear enough what happens. It's not at all clear it's going to happen and it's frustrating to get moved elsewhere as a consequence of an accidental cycle or simply not seeing a containing scope.

[...] "cycles through them backwards"?

Nah, the behaviour is actually nondeterministic. It doesn't seem to be strictly a browser issue and refreshing seems to be able to make it go away. It's not evidently tied to clicking the discs vs. the mutated code, and I think I've seen it cycle rightwards but start from the rightmost disc. I have not definitively witnessed it cycle leftwards.

Yes, the code jumps around a bit, but there is not a lot we can do about that. I also think it isn't that big of a deal. Example:

Throw in a few more mutations on the same line and you'll see what I mean. That nothing can be done about it presupposes that the content must shift, which of course is a viewpoint I feel that the old report adequately demonstrated is untrue.

@feckertson
Copy link
Author

@nicojs

is this what you mean with "cycles through them backwards"?

No. Click on the word "amount" in line 25 of this report several times. That word happens to be included in three different overlapping mutations. Clicking it repeatedly cycles through each of them. Does the cycle not seem backwards to you? It does to me because I read left-to-right and top-to-bottom, not the other way around. This behavior is neither random nor browser dependent.

jumps all over the place

Although this comment was made by someone else, I can appreciate it. Observe that in the previous exercise you could simply click repeatedly on the word "amount" without ever moving the cursor. Now try to click repeatedly on the word "amount" in line 29 (while any mutant from the previous exercise is still active). It cannot be done without repositioning the cursor. From a UX perspective that is poor behavior. A click target should not move away when one clicks on it.

Perhaps it cannot be addressed in all cases but it would be better to have the the scroll position change than to have the last click target jump away from the cursor.

This is heavily debated in....

The sentiment in #1578 is that the original placement of the markers conveyed a huge amount of information and that the underlines do not come close to making up for the loss of that information created by moving the markers to the ends of the lines. I agree.

The sentiment in that conversation is that "a true diff" does not provide that much value, certainly not enough to justify making sacrifices. Besides why is the diff more "true" when the indicators are at the end of the line? The diff still does not have indicators that the original line does have.

The ID should be a technical ID, not really something to communicate

Perhaps, but in the original report they were numbered rectangles. Developers could communicate with each other about a particular mutant in a specific report simply by referencing its number. Now the best one can do is say something like "Hey. I'm having trouble understanding the third red dot on line 275. Can you help me out?"

@commonquail
Copy link

Firefox and Edgeium:

image

@hugo-vrijswijk
Copy link
Member

The diff renderer can be really janky

What do you mean by this? I'm not sure what the performance recording I'm looking at is showing. In the second example toggleMutant takes about 160ms which I think is still pretty fast. It looks like stryker-net is also on an older version of the report, while there have been some performance improvements in a later version. I'll be more than happy to take a look at any performance issues

@commonquail
Copy link

commonquail commented Feb 4, 2022

is this what you mean with "cycles through them backwards"?

No. Click on the word "amount" in line 25 of this report several times. That word happens to be included in three different overlapping mutations. Clicking it repeatedly cycles through each of them. Does the cycle not seem backwards to you? It does to me because I read left-to-right and top-to-bottom, not the other way around. This behavior is neither random nor browser dependent.

I think I figured it out.

I have an unshareable report with a component that behaves in exactly this way and another component that cycles strictly left-to-right, both right next to each other.

The behaviour is deterministic, and browser independent, but I think it is not technically backwards, even though this example clearly feels exactly backwards.

If you click repeatedly on L25's drink => you will cycle through the left-most disc in L25 and the single disc in L24. If you click anywhere to the left of that you will only cycle through L24's single disc. If you click on drink.amount > 0 you will toggle L25's right-most disc, then cycle through to the left-most disc and finally L24's single disc.

What really happens is that the scope of overlapping mutations increases and the toggle cycles from smallest to largest scope. The sense of nondeterminism and reversed cycling comes from overlapping scopes, often with small surface areas, that cannot in any way be distinguished.

Another thing I learned explaining the above is that the toggle handler clears selected text, anywhere in the report, every time you toggle something. This makes it ~impossible to copy any text from the diff view.

I'm not sure what the performance recording I'm looking at is showing. In the second example toggleMutant takes about 160ms which I think is still pretty fast.

The first picture shows toggling the same mutation twice, the screen freezing for about 70ms each time. The second picture shows toggling another mutation, which happened to be larger, once, with the screen freezing for about 300ms and recovering for another 300ms. In the second picture, toggleMutant takes about 280ms, not 160ms, and is framed by other, freezing operations.

To put that into perspective, smooth movement is >=60 FPS, which is 1 frame every <17ms. 70ms may be a small amount of wall-clock time but in interactivity that feels like an eternity.

The report I recorded those one was generated with Stryker.NET 1.3.1. Some quick testing on the place-order-component.js report linked above suggests that the typical experience is closer to 10ms per toggle. That's fast enough to meet 60 FPS but slow enough to leave almost no room for CPU contention. It's highly likely that my CPU happened to be busy at the time (I didn't check) and that upstream changes would have degraded more gracefully.

But I can literally see the input latency in the toggling in the GIF shown above.

@feckertson
Copy link
Author

The sense of nondeterminism and reversed cycling comes from overlapping scopes

Agreed that it can be confusing and that the confusion did not exist before... back when there were clearly numbered markers embedded within the code. One could look at a marker and make inferences about its meaning based on its position. In cases where the meaning was not completely clear one could click one the marker and see its exact meaning. A superior UX design.

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

No branches or pull requests

4 participants