Closed Bug 1385791 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Console, defect, P1)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: nchevobbe, Assigned: Honza)

Details

(Whiteboard: [reserve-console-html])

Attachments

(4 files)

Bug 1385791 - New stubs for tests;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1385791 - Fix message filtering;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1385791 - Babel support for async func;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1385791 - Fix tests;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
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.
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [console-html]
Same thing happens for "XHR" messages too.
Whiteboard: [console-html] → [console-html] [triage]
Priority: P2 → P3
QA Contact: iulia.cristescu
Whiteboard: [console-html] [triage] → [reserve-console-html]
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
See also this comment: bug 1375778 comment #14

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
Debug doesn't seem to work either.

Honza
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 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-
(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
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
Comment on attachment 8895762 [details]
Bug 1385791 - New stubs for tests;

https://reviewboard.mozilla.org/r/167094/#review172658
Attachment #8895762 - Flags: review?(nchevobbe) → 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+
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+
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 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 on attachment 8895762 [details]
Bug 1385791 - New stubs for tests;

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

Honza
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]
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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.