1596093 - Update tests for Rules view to use getTextProperty()

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, task, P3)

Description

[Razvan Caliman [:rcaliman]]

The patch for Bug 1593944 introduced a helper test method called getTextProperty() with which the corresponding TextProperty for a CSS declaration can be obtained.

Many of the tests in devtools/client/inspector/rules/test/ still rely on directly looking up the TextProperty like so:

const rule = getRuleViewRuleEditor(ruleView, 1).rule;
const prop = rule.textProps[0];

These tests can be updated to use the helper instead:

getTextProperty(ruleView, ruleIndex, declaration);

Comment 1

[Patrick Brosset <:pbro>]

For people interested in helping out, this might be a simple enough bug to get started if you know a little bit of JS.
You can take a look at our contribution docs here: https://docs.firefox-dev.tools
Once you're set up, start by finding the places in the code that rely on the old code pattern referenced in comment 0, and change them with getTextProperty.
To test out your changes, you just need to run the tests you change locally which you can do with mach test devtools/client/inspector/rules/test/the_test_you_changed.js (but there's more information about tests here https://docs.firefox-dev.tools/tests/

Comment 2

[Miguel]

I would like to work on this

Comment 3

[Razvan Caliman [:rcaliman]]

Be sure to wait for Bug 1593944 to land in mozilla-central repository first (should happen later today). It contains the getTextProperty() helper.
You will find it in devtools/client/inspector/rules/test/head.js once you pull the updated repository.

Comment 4

[Patrick Brosset <:pbro>]

Thanks Miguel, I'm assigning the bug to you now.

Comment 5

[Patrick Brosset <:pbro>]

Hi Miguel, are you still interested in working on this bug?
If so, please let us know if you need any help at all.

Comment 6

[Aggelos Gamvrinos [:agam]]

Hey Patrick. I can take a look. Can you assign this to me?

Comment 7

[Aggelos Gamvrinos [:agam]]

I took a look and got a bit confused about the suitability of this new function for the needs of most of the tests.

It seems that the getTextProperty(ruleView, ruleIndex, declaration); function does not take the idIndex as parameter but only the ruleIndex. Then, within the function, the latter is used to get the textProps array, which gets searched based on the declaration passed. However, many tests check whether the index of the css property is correct or not, something that is not returned by the getTextProperty invocation.

Example:
Test file: browser_rules_custom.js

  const idRule = getRuleViewRuleEditor(view, 1).rule;
  const idRuleProp = idRule.textProps[0];  // <- property index 0
   ...
   is(
     idRuleProp.name,
     "--background-color",
     "First ID prop should be --background-color"
   );

Initially, I thought that I would need to pass the declaration (e.g. {"--background-color": "blue"} for the example above) to getTextProperty and then maybe assess the returned index in the test rule. However, that doesn't seem to be the case. Any pointers on this?

Comment 8

[Mahak]

Attachment: Bug 1596093 - Update tests for rule view to use getTextProperty()

Comment 9

[Mahak]

@Patrick I have submiited the patch .
Can you please review it?

Comment 10

[Patrick Brosset <:pbro>]

Mahak, it would be have been nice of you to ask Aggelos first before submitting the patch! Aggelos had obviously started to work on it and was asking a question here. Even though the bug was not assigned to them yet, they had clearly asked and had started to give some thoughts into it.
Please refrain from working on bugs that area already being worked on in the future.

Razvan will be reviewing the patch attached here by Mahak. I've assigned the review request to him.

Comment 11

[Mahak]

Patrick , I apologise for this and I'll take care that I'll not repeat this mistake in future.

Comment 12

[Razvan Caliman [:rcaliman]]

Thanks for looking into this, Aggelos!

Indeed, the new method isn't universally applicable. There are tests like the one you mentioned where the goal is to test the value of the property itself. In those cases, rewriting the tests to use the getTextProperty() is of little use.

There are, however, other tests where the property is just a helper in the grander scheme of the test, not the thing being tested. In those cases, the getTextProperty() method can help simplify the test and reduce cognitive load when trying to understand which CSS declaration (aka TextProperty) it is operating on.

For example, in devtools/client/inspector/changes/test/browser_changes_declaration_disable.js:

- const rule = getRuleViewRuleEditor(ruleView, 1).rule;
- const prop = rule.textProps[0];
+ const prop = getTextProperty(ruleView, 1, {"color":"red"})

  let onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
  info("Disable the first declaration");
  await togglePropStatus(ruleView, prop);

The right way to approach this task is to use critical thinking, as you have done, to go over all tests which make use of getRuleViewRuleEditor() and judge whether the property is the thing being tested or not. If it's just a helper, perhaps it helps to simplify by using the getTextProperty() method instead. A good hint will also be to look out whether the rule from getRuleViewRuleEditor() is used for something else as well. That's an indicator that the simplification is not really helpful.

It is a big task because there are many tests to check and understand. You may find it easier to split this work into multiple patches and update a few tests at a time (we can keep the bug open until we can say that most tests which could have been simplified have been handled).

The goal of addressing this is to simplify tests wherever possible to make it easier to understand which CSS declaration (aka TextProperty) the test is referring to. The side-effect is encapsulating some of the specific logic, like getRuleViewRuleEditor(), into the helper so it's easier to refactor tests in the future if we move away from this approach of obtaining the TextProperty.

The benefit of taking on this bug (or parts of it) for you is that reading through those tests will expose you to a lot of the functionality implemented in the Rules panel. This will make it easier to take on other bugs in that area knowing which tests handle them.

Let me know if you have further questions or need more guidance.

Mahak's current patch is incomplete. I have directed them to reading this comment for detail.

Given the scale of the task, you could both work on this if you choose to do so. You will need to coordinate on which tests to take each so you don't duplicate work.

Comment 13

[Mahak]

Hey Aggelos !
I would Love to work with you and I do apologise for not asking before submitting the patch.
It would be great if you can divide this work and I shall be happy to follow that
Thanks in Advance : )

Comment 14

[Aggelos Gamvrinos [:agam]]

Hey Razvan, thanks for the clarification. I see what you mean.

Mahak, sure we can work on this together. I'll split the tests that need to potentially be changed in half and let you know on which split I'll work on.

Just hold on until I get back to you (!)

Comment 15

[Aggelos Gamvrinos [:agam]]

Attachment: batch1_21.PNG

Comment 16

[Aggelos Gamvrinos [:agam]]

Attachment: batch2_31.PNG

Comment 17

[Aggelos Gamvrinos [:agam]]

Hey,

So according to a quick search there are 240 .textProps[... instances in 108 files that need to be checked. Mahak, I split the files and took a screenshot of the filenames that correspond to you. Next to the filenames you can see the no. of .textProps[... instances within that file.

I'll work on the rest (not shown in the attachments).

Let me know if this is fine by you.

Comment 18

[Mahak]

Yeah !!
That's perfect
Thanks for this : )

Comment 19

[Mahak]

I just want to ask How did you sorted the files according to the number of .textProps[.. ??

Comment 20

[Mahak]

(In reply to Razvan Caliman [:rcaliman] from comment #12)

Thanks for looking into this, Aggelos!

Indeed, the new method isn't universally applicable. There are tests like the one you mentioned where the goal is to test the value of the property itself. In those cases, rewriting the tests to use the getTextProperty() is of little use.

There are, however, other tests where the property is just a helper in the grander scheme of the test, not the thing being tested. In those cases, the getTextProperty() method can help simplify the test and reduce cognitive load when trying to understand which CSS declaration (aka TextProperty) it is operating on.

For example, in devtools/client/inspector/changes/test/browser_changes_declaration_disable.js:

- const rule = getRuleViewRuleEditor(ruleView, 1).rule;
- const prop = rule.textProps[0];
+ const prop = getTextProperty(ruleView, 1, {"color":"red"})

  let onTrackChange = waitUntilAction(store, "TRACK_CHANGE");
  info("Disable the first declaration");
  await togglePropStatus(ruleView, prop);

The right way to approach this task is to use critical thinking, as you have done, to go over all tests which make use of getRuleViewRuleEditor() and judge whether the property is the thing being tested or not. If it's just a helper, perhaps it helps to simplify by using the getTextProperty() method instead. A good hint will also be to look out whether the rule from getRuleViewRuleEditor() is used for something else as well. That's an indicator that the simplification is not really helpful.

It is a big task because there are many tests to check and understand. You may find it easier to split this work into multiple patches and update a few tests at a time (we can keep the bug open until we can say that most tests which could have been simplified have been handled).

The goal of addressing this is to simplify tests wherever possible to make it easier to understand which CSS declaration (aka TextProperty) the test is referring to. The side-effect is encapsulating some of the specific logic, like getRuleViewRuleEditor(), into the helper so it's easier to refactor tests in the future if we move away from this approach of obtaining the TextProperty.

The benefit of taking on this bug (or parts of it) for you is that reading through those tests will expose you to a lot of the functionality implemented in the Rules panel. This will make it easier to take on other bugs in that area knowing which tests handle them.

Let me know if you have further questions or need more guidance.

Mahak's current patch is incomplete. I have directed them to reading this comment for detail.

Given the scale of the task, you could both work on this if you choose to do so. You will need to coordinate on which tests to take each so you don't duplicate work.

Hey !!
After reading this I know what to do almost but I still have a doubt regarding the declaration parameter in getTextProperty()
for Example in browser_rules_edit-variable.js

info("Check the initial state of the --color variable");
  checkCSSVariableOutput(
    view,
    "div",
    "color",
    "ruleview-variable",
    "--color = lime"
  );

  info("Edit the CSS variable");
  const prop = getTextProperty(view,1,{"color":"lime"});
  const propEditor = prop.editor();
  const editor = await focusEditableField(view, propEditor.valueSpan);
  editor.input.value = "blue";
  const onRuleViewChanged = view.once("ruleview-changed");
  EventUtils.synthesizeKey("VK_RETURN", {}, view.styleWindow);
  await onRuleViewChanged;
  checkCSSVariableOutput(
    view,
    "div",
    "color",
    "ruleview-variable",
    "--color = blue"
  );
});

Are the changes correct or what should I do in place of color : lime ?

Comment 21

[Razvan Caliman [:rcaliman]]

Hi Aggelos,

Thanks for taking the lead and coordinating on this!

---

Hi Mahak,

There are at least two mistakes in that code which cause that test to fail:

1. The CSS declaration you want to get is the CSS variable, --color: lime, not the color declaration, color: lime which does not exist in that test document's stylesheet.

Notice the index in the textProps array:

const propEditor = ruleEditor.rule.textProps[1].editor;

The number 1 is referring to the second TextProperty instance in that array, therefore the --color: lime declaration. This is a good example of why we want to simplify and use getTextProperty() with the exact CSS declaration instead of an index, so we avoid this confusion in the future.

- const prop = getTextProperty(view, 1, {"color":"lime"});
+ const prop = getTextProperty(view, 1, {"--color":"lime"});

2. The editor is a property of the TextProperty object, not a method.

Running the test yields this error message:

FAIL Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_edit-variable.js:34 - TypeError: prop.editor is not a function

- const propEditor = prop.editor();
+ const propEditor = prop.editor;

Comment 22

[Aggelos Gamvrinos [:agam]]

Hey Razvan,

I've been working on some tests and noticed that for some the textProps array is accessed multiple times based on the same ruleEditor instance. Replacing these with getTextProperty will result in getRuleViewRuleEditor getting called multiple times instead of one. This results in redundant accesses but I guess since this is just for the tests, it is not a big issue. What do you think?

Example:

const ruleEditor = getRuleViewRuleEditor(view, 1);
...
ruleEditor.rule.textProps[0].editor.nameSpan
ruleEditor.rule.textProps[2].editor.nameSpan
ruleEditor.rule.textProps[3].editor.nameSpan
...

Comment 23

[Mahak]

Attachment: Bug 1596093 - Update tests for Rules view to use getTextProperty(). ?rcaliman

Comment 24

[Aggelos Gamvrinos [:agam]]

Attachment: Bug 1596093 - Update tests for Rules view to use getTextProperty(). r=rcaliman

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cc38aea9832
Update tests for Rules view to use getTextProperty(). r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/417f4586c197
Update tests for Rules view to use getTextProperty(). ?rcaliman r=rcaliman

Comment 26

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7cc38aea9832
https://hg.mozilla.org/mozilla-central/rev/417f4586c197

Comment 27

[Gabriel Luong [:gl] (ΦωΦ)]

Really glad to see these changes happening. Thank you both Aggelos and Mahak!