WIP: Auto rules tests #2906

Ambrevar · 2023-04-14T10:51:46Z

Description

This adds the long awaited auto-rules test. Since it's currently broken (see #2895), we are in urgent need of a test suite to validate the many edge cases.

Should help with #2895.

Discussion

Just a draft for now, there is much more to test.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

Git hygiene:
- I have pulled from master before submitting this PR
- There are no merge conflicts.

I've added the new dependencies as:

ASDF dependencies,

Git submodules,

cd /path/to/nyxt/checkout
git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser

and Guix dependencies.

My code follows the style guidelines for Common Lisp code. See:
- Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
- Google Common Lisp Style Guide
I have performed a self-review of my own code.
My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
Documentation:
- All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
- I have updated the existing documentation to match my changes.
- I have commented my code in hard-to-understand areas.
- I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
- I have added a migration.lisp entry for all compatibility-breaking changes.
- (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
Compilation and tests:
- My changes generate no new warnings.
- I have added tests that prove my fix is effective or that my feature works. (If possible.)
- I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

Ambrevar · 2023-04-14T11:34:01Z

@aartaka The auto-rules-remembered-modes last 2 assertions are failing. In my understanding, this is an auto-rules bug. Could you look into it?

aartaka · 2023-04-14T11:39:14Z

@aartaka The auto-rules-remembered-modes last 2 assertions are failing. In my understanding, this is an auto-rules bug. Could you look into it?

Yeah, that looks like no-rule -> no-rule transition, which currently unconditionally restores the last active modes over the enabled ones.

aartaka · 2023-04-14T12:09:59Z

I should've documented auto rules better 😰

Ambrevar · 2023-04-14T14:54:42Z

Commits f22395b and e877455 fix an issue with cl:pathname in quri:uri path.

I believe however that the fix belongs upstream. See fukamachi/quri#78.

aartaka · 2023-04-19T15:45:24Z

faa9f8e fixes the test before the last one. But

the last test still fails,
and transitioning from rule-less page to rule-having one and then back makes the rule-toggled modes to be saved to last-active-modes. Investigating.

aartaka · 2023-04-21T15:25:32Z

I've fixed it, I think. But I'll have to test it more.

In the meantime, here's a full matrix of cases we have to test (with combinations that we currently have marked with names of tests including them):

| transition\rule    | exclude | include              | exclude+include |
| rule -> rule       |         |                      |                 |
| no rule -> no rule |         | remembered-modes + 1 |                 |
| rule -> no rule    |         | navigation... + 1    |                 |
| no rule -> rule    |         | basic                |                 |

But this framework is insufficient to describe all the scenarios. Additional situations to test too (and that current tests more or less manage):

Reloading the page (rule, no rule).
Manual mode toggling (same matrix).

aadcg · 2023-04-21T15:31:56Z

Do we really need the renderer to test auto-rules?

Ambrevar · 2023-04-24T09:24:57Z

@aadcg For now, yes, because auto-rules are triggered by on-signal-decide-policy.

@aartaka Could we trigger auto-rules else where?

Better:

Either we should have a way to load pages without having to render them. But does WebKit allow this? If not, can we decouple resource loading from rendering?
Tweak the dummy renderer to support "loading" HTML, fire on-signal-decide-policy, and thus trigger the auto-rules.

Ambrevar · 2023-04-24T09:25:21Z

@aartaka Great work with the matrix, I'll get down to it.

aadcg · 2023-04-24T11:19:35Z

@Ambrevar makes sense, thanks for the clarification!

aartaka · 2023-04-24T12:05:10Z

@aartaka Great work with the matrix, I'll get down to it.

Note that not all the cases are a huge priority. I'd focus on include and exclude ones, possibly ignoring the include+exclude ones as a supposed subset of the former two.

Some corner-cutting could be done too. Don't get burned out doing several dozens of tests :P

aartaka · 2023-04-24T12:07:00Z

A way to cut some corners: test all the page transitions with just one rule (like we are doing now), and then test all the rule types with some typical use scenario (like no-rule -> rule -> no-rule transition). It's not exactly bullet-proof, but possible to implement fast and covers the most matrix space with least effort spent.

Ambrevar · 2023-04-26T08:02:46Z

source/auto-rules.lisp

+  (and url
+       (or (null (last-active-modes-url buffer))
+           (not (quri:uri= url (last-active-modes-url buffer)))
+           (not (matching-auto-rules (previous-url buffer) buffer)))))


Can you add a docstring to explain what this does?

Ambrevar · 2023-04-26T08:07:56Z

@aartaka Can you also document:

the logic behind top-level-p in gtk.lisp
save-last-active-modes
reapply-last-active-modes (which are the "last" active modes? Consider the case of matched rule and unmatched rule)
remember-on-mode-toggle: Why is save-last-active-modes called if prompt-on-mode-toggle-p is NIL and not in the other branch?
add-modes-to-auto-rules: Docstring including an example usage that involves url-infer-match and rememberable-of.

Ambrevar · 2023-04-26T08:58:18Z

Commit 294b2c2 with mode exclusion fails. @aartaka Can you look into it?

aartaka · 2023-04-27T15:00:51Z

the logic behind top-level-p in gtk.lisp

See the new documentation in toplevel-p slot of request-data.

save-last-active-modes

reapply-last-active-modes (which are the "last" active modes? Consider the case of matched rule and unmatched rule)

remember-on-mode-toggle: Why is save-last-active-modes called if prompt-on-mode-toggle-p is NIL and not in the other branch?

See the updated docs and last-active-modes slot documentation.

add-modes-to-auto-rules: Docstring including an example usage that involves url-infer-match and rememberable-of.

I'm wondering if url-infer-match should actually be part of add-modes-to-auto-rules...

No cache is retained, so history is empty again on subsequent run.

Indeed, Quri allows cl:pathname as path.

This ensures that all the cases with non-matching pages save/restore last-active-modes.

Two changes: - last-active-modes are saved if there was no previous rules. - And they are reapplied when going between two pages with rules.

aartaka · 2023-04-28T13:36:07Z

Commit 294b2c2 with mode exclusion fails. @aartaka Can you look into it?

Two of the failures were due to a typo (fixed in 6ec8133) :P

The third one... Not sure. Investigating.

aartaka · 2023-05-01T10:07:20Z

This is extremely weird. Test fails locally, but the interactive session works just fine (・_・ヾ

Backported from #2636.

aadcg · 2023-05-09T09:33:52Z

What's the status here?

aartaka · 2023-05-09T12:24:35Z

Partially fixed on master, but needs more tests (and, supposedly, more fixes).

aartaka · 2023-05-09T12:25:24Z

What's the status here?

Partially fixed on master, but needs more tests (and, supposedly, more fixes).

aadcg · 2023-05-09T12:45:24Z

I think this is not the right way of handling communication in a collaborative project.

If you have merged a subset of commits featured in this PR, you should have informed about it beforehand in this thread.

Then you should inform the parties involved about the future of this PR. Should it be closed, rebased, etc? Loose ends are enemies of a successful project.

Partially fixed on master, but needs more tests (and, supposedly, more fixes).

What should we do about this PR?

Ambrevar · 2023-05-10T08:02:12Z

We need to finish it, as the tests (about mode exclusion) do not all pass. See comment #2906 (comment).

jmercouris · 2023-10-28T22:02:11Z

Please reopen when ready.

aadcg · 2023-10-30T20:25:38Z

This PR personifies the kind of interaction we can't tolerate. See my comment above.

The fact that auto rules was deployed without a test suite in the first place is also a bad practice.

Ambrevar force-pushed the auto-rules-tests branch from b369555 to fa685d0 Compare April 14, 2023 11:33

Ambrevar mentioned this pull request Apr 26, 2023

Allow dummy renderer to load HTML, then use it in tests #2932

Open

Ambrevar commented Apr 26, 2023

View reviewed changes

Ambrevar and others added 12 commits April 28, 2023 16:48

tests/offline/global-history: Fix reproducibility.

40b1ac1

No cache is retained, so history is empty again on subsequent run.

urls: Fix url-equal when URL path is a cl:pathname.

10ebeb4

Indeed, Quri allows cl:pathname as path.

urls: Fix empty-path-url-p when URL path is a cl:pathname.

19ddc7e

tests: Add 2 extra test pages.

6e231e7

tests: Add auto-rules tests.

db495fd

auto-rules: Fix last-active-modes not always being saved.

065cadc

This ensures that all the cases with non-matching pages save/restore last-active-modes.

mode: Don't remember modes in their togglers.

3a685d2

auto-rules: Fix last-active-modes saving logic yet again.

a6a390d

Two changes: - last-active-modes are saved if there was no previous rules. - And they are reapplied when going between two pages with rules.

auto-rules: Fix typo in auto-rule slot documentation.

bad878c

auto-rules: Add URL keyword argument to saving commands.

c21d1e8

auto-rules: Export rememberable-of and url-infer-match.

ab36eae

tests/renderer-offline/auto-rules: Fix auto-rules-manual-disabling test.

d388857

Ambrevar and others added 6 commits April 28, 2023 16:48

tests/renderer-offline/auto-rules: Add other ways to add rules.

d3775a4

mode: Have mode togglers return the mode object.

3c83cb8

tests/renderer-offline/auto-rules: Add initial test matrix.

3390a7d

Document toplevel-p logic.

51f6316

auto-rules: Document most functions relating to last-active-modes.

2b5882f

auto-rules(add-modes-to-auto-rules): Document.

93aba2f

aartaka force-pushed the auto-rules-tests branch from 75d3257 to 93aba2f Compare April 28, 2023 12:48

tests/renderer-offline/auto-rules: Fix a typo.

6ec8133

tests/renderer-offline/auto-rules: Add more to the matrix.

3615ba0

aadcg mentioned this pull request May 2, 2023

Testing auto-rules.lisp #2636

Open

aadcg referenced this pull request May 9, 2023

mode,auto-rules: Fix auto-rules logic.

187e378

Backported from #2636.

aadcg assigned Ambrevar and aartaka May 9, 2023

jmercouris closed this Oct 28, 2023

aadcg deleted the auto-rules-tests branch October 30, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Auto rules tests #2906

WIP: Auto rules tests #2906

Ambrevar commented Apr 14, 2023 •

edited

Ambrevar commented Apr 14, 2023

aartaka commented Apr 14, 2023

aartaka commented Apr 14, 2023

Ambrevar commented Apr 14, 2023

aartaka commented Apr 19, 2023

aartaka commented Apr 21, 2023

aadcg commented Apr 21, 2023

Ambrevar commented Apr 24, 2023

Ambrevar commented Apr 24, 2023

aadcg commented Apr 24, 2023

aartaka commented Apr 24, 2023

aartaka commented Apr 24, 2023

Ambrevar Apr 26, 2023

Ambrevar commented Apr 26, 2023 •

edited

Ambrevar commented Apr 26, 2023

aartaka commented Apr 27, 2023

aartaka commented Apr 28, 2023

aartaka commented May 1, 2023

aadcg commented May 9, 2023

aartaka commented May 9, 2023

aartaka commented May 9, 2023 via email

aadcg commented May 9, 2023

Ambrevar commented May 10, 2023

jmercouris commented Oct 28, 2023

aadcg commented Oct 30, 2023

WIP: Auto rules tests #2906

WIP: Auto rules tests #2906

Conversation

Ambrevar commented Apr 14, 2023 • edited

Description

Discussion

Checklist:

Ambrevar commented Apr 14, 2023

aartaka commented Apr 14, 2023

aartaka commented Apr 14, 2023

Ambrevar commented Apr 14, 2023

aartaka commented Apr 19, 2023

aartaka commented Apr 21, 2023

aadcg commented Apr 21, 2023

Ambrevar commented Apr 24, 2023

Ambrevar commented Apr 24, 2023

aadcg commented Apr 24, 2023

aartaka commented Apr 24, 2023

aartaka commented Apr 24, 2023

Ambrevar Apr 26, 2023

Choose a reason for hiding this comment

Ambrevar commented Apr 26, 2023 • edited

Ambrevar commented Apr 26, 2023

aartaka commented Apr 27, 2023

aartaka commented Apr 28, 2023

aartaka commented May 1, 2023

aadcg commented May 9, 2023

aartaka commented May 9, 2023

aartaka commented May 9, 2023 via email

aadcg commented May 9, 2023

Ambrevar commented May 10, 2023

jmercouris commented Oct 28, 2023

aadcg commented Oct 30, 2023

Ambrevar commented Apr 14, 2023 •

edited

Ambrevar commented Apr 26, 2023 •

edited