Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon testing suite #5058

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Addon testing suite #5058

merged 2 commits into from
Nov 29, 2022

Conversation

bakulf
Copy link
Collaborator

@bakulf bakulf commented Nov 28, 2022

This is a way to test addons in functional tests. It works this way:

  • in tests/functional/addons we create a folder for each "scenarios" we want to expose. We can expose a manifest.json directly, or a few folders, one for each addon.
  • the addons are built by our CI before running the tests
  • there is an addon.js service that reads all those addons and exposes them to the functional tests
  • Tests are built to use localhost:3003 as addon custom URL.

I migrated all the tests and I wrote a few more. But we can do more! Here is the list to start:

  • tests for in-app messaging where we simulate different types of messages and how they are displayed, dismissed, etc
  • tests for tutorials - settings rollback, completion modal, etc
  • tests for guides - the rendering of guides
  • tests for the tips&tricks view - we can finally test exactly the list of addons we want to show

@bakulf bakulf requested a review from brizental as a code owner November 28, 2022 13:52
@bakulf bakulf force-pushed the addon branch 8 times, most recently from 46881f9 to a7eb906 Compare November 28, 2022 18:27
@bakulf bakulf requested a review from mozrokafor November 28, 2022 18:52
@bakulf bakulf changed the title Addon testing suite - ignore Addon testing suite Nov 28, 2022
Copy link
Contributor

@mozrokafor mozrokafor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,

Copy link
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some initial comments. I need to give this another pass later, it's dense. Overall I like the way we are going. This is a great improvement for addon testing capabilities!

src/addons/manager/addonmanager.h Outdated Show resolved Hide resolved
src/addons/manager/addonmanager.h Show resolved Hide resolved
src/tasks/addonindex/taskaddonindex.cpp Show resolved Hide resolved
tests/functional/testAddons.js Outdated Show resolved Hide resolved
tests/functional/setupVpn.js Outdated Show resolved Hide resolved
tests/functional/testTutorials.js Outdated Show resolved Hide resolved
tests/functional/testTipsAndTricksIntroModal.js Outdated Show resolved Hide resolved
tests/functional/testAddons.js Outdated Show resolved Hide resolved
tests/functional/servers/addon.js Outdated Show resolved Hide resolved
tests/functional/servers/addon.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2022

Codecov Report

Base: 25.70% // Head: 28.26% // Increases project coverage by +2.56% 🎉

Coverage data is based on head (8db57de) compared to base (b4bc088).
Patch coverage: 6.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5058      +/-   ##
==========================================
+ Coverage   25.70%   28.26%   +2.56%     
==========================================
  Files          12      255     +243     
  Lines         607    14970   +14363     
  Branches      325     8567    +8242     
==========================================
+ Hits          156     4231    +4075     
- Misses        262     4251    +3989     
- Partials      189     6488    +6299     
Flag Coverage Δ
lottie_tests 23.06% <ø> (ø)
unit_tests 28.37% <6.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/constants.h 28.57% <0.00%> (ø)
src/inspector/inspectorhandler.h 0.00% <ø> (ø)
src/settingslist.h 0.00% <0.00%> (ø)
src/tasks/addonindex/taskaddonindex.cpp 0.00% <0.00%> (ø)
tests/unit/testaddonindex.cpp 7.59% <0.00%> (ø)
src/addons/manager/addonmanager.cpp 0.38% <2.38%> (ø)
src/addons/manager/addonindex.cpp 32.72% <14.28%> (ø)
src/addons/manager/addondirectory.cpp 18.18% <75.00%> (ø)
tests/unit/testtasks.cpp 31.70% <0.00%> (ø)
src/tutorial/tutorialstep.cpp 29.59% <0.00%> (ø)
... and 241 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I get it now. I like how clean the addon functional tests are looking 💯 Left just a few more nits and questions. Otherwise this is looking good.

assert(await vpn.isFeatureFlippedOff('addonSignature'));
}
it('Empty addon index', async () => {
await vpn.resetAddons('01_empty_manifest');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it would be better if we reset to the empty manifest on the beforeEach block everytime and then in the test cases we defaulted to fetchAddons. IIUC right now it is necessary to call reset as the first thing in each test case in order not to leak state from one test to the next. Is that correct? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we even do that for ALL tests so that addon changes are never leaked no matter what people do? Not sure there a global beforeEach block, if not just ignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end of any test we call hardReset() that deletes all the settings.

tests/functional/helper.js Outdated Show resolved Hide resolved
json.type === 'reset_addons' && !('error' in json),
`Command failed: ${json.error}`);

await this.waitForCondition(async () => this.lastAddonLoadingCompleted());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. Does the _writeCommand('reset_addons') end up having connect called again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. _writeCommand() writes something on a established connected channel (websocket for functional tests, wasm JS for wasm).

@bakulf bakulf requested a review from brizental November 29, 2022 18:40
@bakulf bakulf merged commit 24207f3 into main Nov 29, 2022
@bakulf bakulf deleted the addon branch November 29, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants