Add mocha test for messages "store cleaning" when messages are pruned

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: Console
P1
minor
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

unspecified
Firefox 56
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [console-html])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
There's a limit of messages in the store.
When the limit is hit, oldest messages are removed from the redux store.

Here "being removed" covers multiple things :
- The message object is removed from the `messagesById` map
- The message id is removed from the `visibleMessages` array
- The message id is removed from the `messagesUiById` list
- The object matching the message id is removed from the `messagesTableDataById` map
- The object matching the message id is removed from the `groupsById` map
- The object matching the message id is removed from the `repeatById` object

At the moment, the mocha tests only check that some of those properties are handled (messagesById & visibleMessages mainly).

It would be nice to make sure all of the properties does not contain reference to messages that were pruned.
(Assignee)

Updated

6 months ago
Priority: -- → P2
Whiteboard: [console-html]

Updated

6 months ago
Flags: qe-verify?
Whiteboard: [console-html] → [console-html] [triage]
Flags: qe-verify? → qe-verify-

Updated

6 months ago
Whiteboard: [console-html] [triage] → [console-html]
(Assignee)

Updated

6 months ago
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1

Updated

6 months ago
Iteration: --- → 56.1 - Jun 26
Comment hidden (mozreview-request)

Comment 2

6 months ago
mozreview-review
Comment on attachment 8879026 [details]
Bug 1370486 - Add mocha tests for messages store cleaning.

https://reviewboard.mozilla.org/r/150316/#review155022

Looks reasonable.

Thanks for the example on IRC, it explains how the repeat works!

R+

Honza

::: devtools/client/webconsole/new-console-output/reducers/messages.js:335
(Diff revision 1)
>      }, {});
>  
>    record.set("messagesById", record.messagesById.withMutations(cleanUpCollection));
>  
>    if (record.messagesUiById.find(isInRemovedId)) {
> -    record.set("messagesUiById", record.messagesUiById.withMutations(cleanUpCollection));
> +    record.set("messagesUiById", cleanUpList(record.messagesUiById));

This is performance improvement, correct?

::: devtools/client/webconsole/new-console-output/test/store/messages.test.js:167
(Diff revision 1)
> +
> +      messages = getAllMessagesById(getState());
> +      repeats = getAllRepeatById(getState());
> +
> +      // // There should be only the data for the "undefined" message.
> +      expect(Object.keys(repeats)).toEqual([lastMessageId]);

nit: double `//`, remove one

::: devtools/client/webconsole/new-console-output/test/store/messages.test.js:167
(Diff revision 1)
> +
> +      messages = getAllMessagesById(getState());
> +      repeats = getAllRepeatById(getState());
> +
> +      // // There should be only the data for the "undefined" message.
> +      expect(Object.keys(repeats)).toEqual([lastMessageId]);

You could also check number of repeats at this point.

I guess:
`expect(Object.keys(repeats).length).toBe(1);`
Attachment #8879026 - Flags: review?(odvarko) → review+
Comment hidden (mozreview-request)

Comment 4

6 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea7907038c5f
Add mocha tests for messages store cleaning. r=Honza

Comment 5

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea7907038c5f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.