"Requests", "XHR" and "CSS" filtering are buggy

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Console
P1
normal
VERIFIED FIXED
4 months ago
4 months ago

People

(Reporter: nchevobbe, Assigned: Honza)

Tracking

Trunk
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-console-html])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

4 months ago
Steps to reproduce:

Go to mozilla.org
Open the console
Open the filter bar
Select only "logs" and "Requests"
Reload the page (you should see requests messages)
Turn off the "Logs" button

Expected results:
The requests message are still visible.

Actual results:
Nothing is displayed

This is a bug in our filtering logic that should be fixed and tested.
(Reporter)

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [console-html]
(Reporter)

Comment 1

4 months ago
Same thing happens for "XHR" messages too.

Updated

4 months ago
Whiteboard: [console-html] → [console-html] [triage]

Updated

4 months ago
Priority: P2 → P3
QA Contact: iulia.cristescu
Whiteboard: [console-html] [triage] → [reserve-console-html]
(Reporter)

Comment 2

4 months ago
A similar thing happens for the "CSS" filter: if you don't have the "Warning" filter on, you don't see the CSS messages
Summary: When filter is on only for "Requests", requests are not visible → "Requests", "XHR" and "CSS" filtering are buggy
(Assignee)

Comment 3

4 months ago
See also this comment: bug 1375778 comment #14

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED

Updated

4 months ago
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
(Assignee)

Comment 4

4 months ago
Debug doesn't seem to work either.

Honza
Comment hidden (mozreview-request)
(Assignee)

Comment 6

4 months ago
Patch pushed for review:

Some comments:

- Changes in common.css (fix Filter buttons), message-containe.js and new-console-output-wrapper (avoid NPE) are related to Chrome. This allows to load the Launchpad in Chrome and debug.
- I am working on tests now

Honza
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

4 months ago
mozreview-review
Comment on attachment 8895762 [details]
Bug 1385791 - New stubs for tests;

https://reviewboard.mozilla.org/r/167094/#review172244

The patch looks good (shouldMessageBeVisible is really easier to read).
We should have tests for all those filters though, and there are some things in the patch that were probably added by mistake :)

::: devtools/client/webconsole/new-console-output/reducers/messages.js:434
(Diff revision 3)
> +/**
> + * Returns true if given message should be visible, false otherwise.
> + */
>  function shouldMessageBeVisible(message, messagesState, filtersState, checkGroup = true) {
> -  return (
> -    (
> -      checkGroup === false
> -      || isInOpenedGroup(message, messagesState.groupsById, messagesState.messagesUiById)
> -    )
> -    && (
> -      isUnfilterable(message)
> -      || (
> -        matchLevelFilters(message, filtersState)
> -        && matchCssFilters(message, filtersState)
> -        && matchNetworkFilters(message, filtersState)
> -        && matchSearchFilters(message, filtersState)
> -      )
> -    )
> -  );
> +  // Do not display the message if it's in closed group.
> +  if (checkGroup && !isInOpenedGroup(message, messagesState.groupsById,
> +      messagesState.messagesUiById)) {
> +    return false;
> +  }
> +
> +  // Some messages can't be filtered out (e.g. groups).
> +  // So, always return true for those.
> +  if (isUnfilterable(message)) {
> +    return true;
> +  }
> +
> +  // Return true if the message belongs to enabled type
> +  // ('log', 'warn', 'info', 'error', 'debug').
> +  // These message come from console API calls.
> +  if (matchTypeFilters(message, filtersState)) {
> +    return true;
> +  }
> +
> +  // Return true if the message type is 'debug'
> +  // and Debug filter is enabled.
> +  if (matchDebugFilters(message, filtersState)) {
> +    return true;
> +  }
> +
> +  // Return true if the message source is 'css'
> +  // and CSS filter is enabled.
> +  if (matchCssFilters(message, filtersState)) {
> +    return true;
> +  }
> +
> +  // Return true if the message is network event
> +  // and Network and/or XHR filter is enabled.
> +  if (matchNetworkFilters(message, filtersState)) {
> +    return true;
> +  }
> +
> +  // Return true if the user is filtering using a free text
> +  // and the message contains it.
> +  if (matchSearchFilters(message, filtersState)) {
> +    return true;
> +  }
> +
> +  return false;

ahah, I almost have the same change in my patch for hidden messages :)

::: devtools/client/webconsole/new-console-output/reducers/messages.js:439
(Diff revision 3)
> +/**
> + * Returns true if given message should be visible, false otherwise.
> + */
>  function shouldMessageBeVisible(message, messagesState, filtersState, checkGroup = true) {
> -  return (
> -    (
> +  // Do not display the message if it's in closed group.
> +  if (checkGroup && !isInOpenedGroup(message, messagesState.groupsById,

nit: the test is a bit hard to parse, could we try 
```
if (
  checkGroup 
  && !isInOpenedGroup(message, messagesState.groupsById, messagesState.messagesUiById)
)
```

::: devtools/client/webconsole/new-console-output/reducers/messages.js:457
(Diff revision 3)
> +  // Return true if the message type is 'debug'
> +  // and Debug filter is enabled.
> +  if (matchDebugFilters(message, filtersState)) {
> +    return true;
> +  }

looking at the function, I think everything is already covered in MatchTypeFilters, or am I missing something ?

::: devtools/client/webconsole/package.json:10
(Diff revision 3)
>      "node": ">=6.9.0"
>    },
>    "scripts": {
>      "start": "cross-env NODE_ENV=production node bin/dev-server",
>      "dev": "node bin/dev-server",
> -    "test": "cross-env NODE_ENV=test NODE_PATH=../../../ mocha new-console-output/test/**/*.test.js --compilers js:babel-register -r jsdom-global/register -r ./new-console-output/test/require-helper.js"
> +    "test": "cross-env NODE_ENV=test NODE_PATH=../../../ mocha new-console-output/test/**/filters.test.js --compilers js:babel-register -r jsdom-global/register -r ./new-console-output/test/require-helper.js"

I think this is not meant to be commited.

FYI, if you want to run only one mocha test, you can use `.only` (https://mochajs.org/#exclusive-tests)
Attachment #8895762 - Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 months ago
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> nit: the test is a bit hard to parse, could we try 
> ```
> if (
>   checkGroup 
>   && !isInOpenedGroup(message, messagesState.groupsById,
> messagesState.messagesUiById)
> )
> ```
Done

> looking at the function, I think everything is already covered in
> MatchTypeFilters, or am I missing something ?
I changed the logic a bit and it's needed (especially for assert that should be hidden behind Debug filter too).

> FYI, if you want to run only one mocha test, you can use `.only`
> (https://mochajs.org/#exclusive-tests)
Nice!

Thanks for the review!

Honza
(Assignee)

Comment 15

4 months ago
There are four patches:

* Bug 1385791 - New stubs for tests; // New stubs for tests
* Bug 1385791 - Fix message filtering; // The actual fix for this report
* Bug 1385791 - Babel support for async func; // I had to do this to fix babel on win
* Bug 1385791 - Fix tests; // Fixing & adopting existing tests + new for 'info'

Honza
(Reporter)

Comment 16

4 months ago
mozreview-review
Comment on attachment 8895762 [details]
Bug 1385791 - New stubs for tests;

https://reviewboard.mozilla.org/r/167094/#review172658
Attachment #8895762 - Flags: review?(nchevobbe) → review+
(Reporter)

Comment 17

4 months ago
mozreview-review
Comment on attachment 8896173 [details]
Bug 1385791 - Fix message filtering;

https://reviewboard.mozilla.org/r/167436/#review172660

r+ with the change on debug messages, which I think will also impact stubs.

::: devtools/client/webconsole/new-console-output/reducers/messages.js:452
(Diff revision 1)
> -        && matchCssFilters(message, filtersState)
> -        && matchNetworkFilters(message, filtersState)
> -        && matchSearchFilters(message, filtersState)
> -      )
> -    )
> -  );
> +  // So, always return true for those.
> +  if (isUnfilterable(message)) {
> +    return true;
> +  }
> +
> +  // Let's check all filters and hide the message if they say so.

nit: s/if they say so./if it doesn't match.

::: devtools/client/webconsole/new-console-output/reducers/messages.js:457
(Diff revision 1)
> -  );
> +  // Let's check all filters and hide the message if they say so.
> +  if (!matchFilters(message, filtersState)) {
> +    return false;
> +  }
> +
> +  // If there is a free text available for filtering use it to decide

nit: 
If there is a free text available for filtering…
→
If there is a non-empty text filter…

::: devtools/client/webconsole/new-console-output/reducers/messages.js:522
(Diff revision 1)
> -  return filters.get(message.level) === true;
> +  // Some messages are using a level that shouldn't be actually
> +  // used for filtering.
> +  // An example is a 'debug' message. It has level 'log', but
> +  // should be displayed only if filter 'debug' is enabled.
> +  // So, make sure to check whether the message type has its
> +  // own filter (like e.g. 'debug') which is explicitly disabled.
> +  if (filters.get(message.type) === false) {
> +    return false;
> +  }

I think we are the one who are doing wrong things in prepareMessage here.
If you look at http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/utils/messages.js#331 , we assign the LOG level to debug messages.
I think this is a bug (I can't find in the file history why it would have been done on purpose) and can be fixed there, so we don't have to treat debug filtering as an exception here.
Attachment #8896173 - Flags: review?(nchevobbe) → review+
(Reporter)

Comment 18

4 months ago
mozreview-review
Comment on attachment 8896174 [details]
Bug 1385791 - Babel support for async func;

https://reviewboard.mozilla.org/r/167438/#review172662
Attachment #8896174 - Flags: review?(nchevobbe) → review+
(Reporter)

Comment 19

4 months ago
mozreview-review
Comment on attachment 8896175 [details]
Bug 1385791 - Fix tests;

https://reviewboard.mozilla.org/r/167440/#review172664

The tests looks good, but I wonder if we could make them more readable and explicit.
By that I mean not calling `prepareBaseStore` in `beforeEach`, but inside each test, dispatch the messages that matters for the test, and turn the filter on and off to make sure the message disappears/appears again.
This could be done as a follow-up to refactor this test file since this patch is about fixing the filtering logic, so r+ for this, and I let you decide if you want to take care of this right-away or in a follow-up

::: devtools/client/webconsole/new-console-output/test/store/filters.test.js:38
(Diff revision 1)
> +   * all filters and consequently tests one by one on the list of messages
> +   * created in `prepareBaseStore` method.
> +   */
>    describe("Level filter", () => {
> +    beforeEach(() => {
> +      // Switch off all filters (include those which are on by default).

nit: this is a bit confusing here because we call clear, and then toggle some filters.

The comment could say : 
Resets all filters to their default values, then
toggle off those which are on by default.
Attachment #8896175 - Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 24

4 months ago
mozreview-review
Comment on attachment 8895762 [details]
Bug 1385791 - New stubs for tests;

https://reviewboard.mozilla.org/r/167094/#review172790

::: devtools/client/webconsole/new-console-output/test/fixtures/stubs/consoleApi.js:567
(Diff revision 5)
> +  "allowRepeating": true,
> +  "source": "console-api",
> +  "timeStamp": 1502371310886,
> +  "type": "debug",
> +  "helperType": null,
> +  "level": "log",

looks like we still have the "log" level here. Have you made the change in getLevel http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/utils/messages.js#331 ? And if so, have you generated the stubs again ?
Attachment #8895762 - Flags: review+ → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

4 months ago
Try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4707e5e53b44bc6ff79710f7351c7e31f08e1a4d

Honza
(Reporter)

Comment 30

4 months ago
mozreview-review
Comment on attachment 8895762 [details]
Bug 1385791 - New stubs for tests;

https://reviewboard.mozilla.org/r/167094/#review173024
Attachment #8895762 - Flags: review?(nchevobbe) → review+
(Reporter)

Comment 31

4 months ago
mozreview-review
Comment on attachment 8896173 [details]
Bug 1385791 - Fix message filtering;

https://reviewboard.mozilla.org/r/167436/#review173026

Comment 32

4 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0157c78fa34
Babel support for async func; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/02c518b4c053
New stubs for tests; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/20f25f9f2874
Fix message filtering; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d365aa793d74
Fix tests; r=nchevobbe
(Assignee)

Comment 33

4 months ago
Cool, thanks!

Honza
https://hg.mozilla.org/mozilla-central/rev/b0157c78fa34
https://hg.mozilla.org/mozilla-central/rev/02c518b4c053
https://hg.mozilla.org/mozilla-central/rev/20f25f9f2874
https://hg.mozilla.org/mozilla-central/rev/d365aa793d74
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have reproduced this bug with nightly 56.0a1 (2017-07-31) on Linux Mint (64 Bit).

The bug's fix is now verified on Latest Nightly 57.0a1

Build ID 	20170814100258
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170809]

Comment 36

4 months ago
I have successfully reproduced this bug with Nightly 56.0a1 (2017-07-31) (32-bit) on windows 10(32bit)

this bug is verified fix with  latest nightly 57.0a1 (2017-08-15) (32-bit)

Build ID: 20170815100349
Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170809]
Managed to reproduce the initial issue on 52.0a1 (2016-10-05). I can confirm the bug is verified fixed on 57.0a1 (2017-08-21), using Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.12. Also based on the previous comments, I will update the flags accordingly.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.