Refactor mwApiPath (to mwActionApiPath), mwRestApiPath, mwModulePath and mwWikiPath parameters + fix regression of test render template #1939

VadimKovalenkoSNF · 2023-10-30T15:40:50Z

Fixes #1935
Depends on #1944

List of fixes:

Comment out VisualEditor capabilities check and tests. It should be enabled back once https://phabricator.wikimedia.org/T350117 is unblocked.
Partially comment out error checking in checkApiAvailability till the issue with VE is resolved.
Fixed regressions around mwApiPath, mwRestApiPath, mwModulePath, mwWikiPath. Updated base director methods.
Renamed mwApiPath to mwActionApiPath.
~~Explicitly set { mwWikiPath: '/' } in the tests to prevent default wiki/ path to assigning.~~ ~~Default value of mwWikiPath is the empty string as per discussion in [REGRESSION?] API end-point detection does not take care properly of command line argument #1935 (comment)~~ Default wikiPath is 'wiki/' as it was before.

~~❗ Important - this will probably affect receipts in the zimfarm because --mwWikiPath='/' is obligated param for the Mediawiki wikis now.~~

~~Check this command as an example: npm start -- --mwUrl=https://en.wikipedia.org --adminEmail=mail@mail.com --articleList=Canada --mwWikiPath=/.~~

~~See #1935 (comment) for more details.~~
^ Upd: this is no longer valid, mwWikiPath can be default 'wiki/'

Refactored regression with test/testAllRenders.ts - fixed renderer handling across e2e tests, fixed issue with non-removed test zim files, added optional render list that is useful for wikis that do not support all renders from RENDERERS_LIST
Optimized src/renderers/renderer.builder.ts to meet Codecov requirements.
Fixed Mediawiki setters side effects when they received undefined parameters.

codecov · 2023-11-03T07:04:52Z

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (87333d0) 75.43% compared to head (99842b0) 73.27%.
Report is 2 commits behind head on main.

❗ Current head 99842b0 differs from pull request most recent head d3b0125. Consider uploading reports for the commit d3b0125 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1939      +/-   ##
==========================================
- Coverage   75.43%   73.27%   -2.16%     
==========================================
  Files          37       39       +2     
  Lines        2984     3106     +122     
  Branches      653      687      +34     
==========================================
+ Hits         2251     2276      +25     
- Misses        633      711      +78     
- Partials      100      119      +19

Files	Coverage Δ
src/mwoffliner.lib.ts	`74.72% <100.00%> (ø)`
src/parameterList.ts	`100.00% <ø> (ø)`
src/util/builders/url/base.director.ts	`85.00% <100.00%> (+0.78%)`	⬆️
src/Downloader.ts	`68.56% <66.66%> (+1.19%)`	⬆️
src/MediaWiki.ts	`85.84% <85.36%> (+1.76%)`	⬆️
src/renderers/renderer.builder.ts	`34.00% <60.00%> (-18.09%)`	⬇️
src/sanitize-argument.ts	`8.08% <33.33%> (ø)`

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kelson42 · 2023-11-03T14:27:14Z

Comment out VisualEditor capabilities check and tests. It should be enabled back once https://phabricator.wikimedia.org/T350117 is unblocked.

Partially comment out error checking in checkApiAvailability till the issue with VE is resolved.

This PR has nothing to do with VE, as such nothing regarding VE should normaly be inside... and for sure not https://phabricator.wikimedia.org/T350117 which has really nothing to do with this ticket.

kelson42

Few changes to do only.

src/util/builders/url/base.director.ts

kelson42 · 2023-11-03T15:24:03Z

src/Downloader.ts

@@ -175,13 +176,13 @@ class Downloader {
      this.baseUrl = basicURLDirector.buildDownloaderBaseUrl([
        { condition: await MediaWiki.hasWikimediaMobileApi(), value: MediaWiki.WikimediaMobileApiUrl.href },
        { condition: await MediaWiki.hasWikimediaDesktopApi(), value: MediaWiki.WikimediaDesktopApiUrl.href },
-        { condition: await MediaWiki.hasVisualEditorApi(), value: MediaWiki.visualEditorApiUrl.href },
+        // { condition: await MediaWiki.hasVisualEditorApi(), value: MediaWiki.visualEditorApiUrl.href },


1 - create a PR to deactivate visualeditor
2 - create a ticket requesting reintroducing it once the phabricator ist fix and rolled out
3 - put links between all of this so things are understandable

Only the VisualEditor autodetection should be deactivated

Done, this PR has been merged - #1941
Ticket about reintroducing VE - #1940

kelson42 · 2023-11-03T15:29:07Z

test/e2e/multimediaContent.test.ts

+            rimraf.sync(`./${outFiles[0].testId}`)
+          })
+          break
+        case 'VisaulEditor':


src/mwoffliner.lib.ts

benoit74 · 2023-11-07T21:25:04Z

I like the fact that you completely removed --mwApiPath CLI param. Reusing it for the REST endpoint (as it is currently done in main branch) was an extremely breaking change from my PoV.

This CLI changes are somehow a breaking change (we cannot set --mwApiPath anymore) but at least we will get a clear error instead of very weird things when the parameter was finally used for something else.

Good job!

VadimKovalenkoSNF · 2023-11-08T08:52:15Z

Thank you, @benoit74 , but I want to emphasize that mwApiPath was renamed to mwActionApiPath. Full list of param definitions can be found in src/parameterList.ts. Moreover, mwWikiPath is obligated now (this basically was introduced in this PR) for scraping wikimedia wikis correctly and mwWikiPath and mwActionApiPath are necessary for 3rd party wikis, see #1935 (comment) for more details.

kelson42

CI does not pass

VadimKovalenkoSNF · 2023-11-10T10:27:26Z

CI does not pass

Because of the regression described in #1943

kelson42 · 2023-11-10T10:28:58Z

test/unit/styles.test.ts

@@ -13,7 +13,7 @@ describe('Styles', () => {

  test('Stylesheet downloading', async () => {
    const { articleDetailXId } = RedisStore
-    const { downloader } = await setupScrapeClasses() // en wikipedia
+    const { downloader } = await setupScrapeClasses({ mwWikiPath: '/' }) // en wikipedia


We only test against Wikimedia Web site, so why not setting this once for all? Why it's not the default value in setupScrapeClasses()?

I fixed this issue.

kelson42 · 2023-11-10T10:35:08Z

test/testAllRenders.ts


  return outFiles
 }

-export async function testAllRenders(parameters: Parameters, callback) {
+export async function testAllRenders(parameters: Parameters, callback, optionalRenderesList?: Array<string>) {


Naming of the method is not appropriate anymore, it's not "all" Renders.

I would recommend to:

Rename this to TestRenders()

Create testAllRenders() with RENDERERS_LIST as argument list for then redirecting to TestRenders()

It's somehow concerning to have e2e tests not running on all renders as this is a basic requirement... but here there is maybe a special use case...

Refactored.

kelson42 · 2023-11-10T10:39:54Z

test/e2e/forceRender.test.ts

@@ -39,7 +40,7 @@ describe('forceRender', () => {
    expect(redisScan.stdout).toEqual('')
  })

-  test('Scrape article from bm.wikipedia.org should throw error when using VisualEditor render', async () => {
+  test.skip('Scrape article from bm.wikipedia.org should throw error when using VisualEditor render', async () => {


why skipping? Actually it should not throw an error, it should work perfectly.

kelson42 · 2023-11-10T10:59:03Z

src/MediaWiki.ts

+
+  private initApiURLDirector() {
+    // TODO: this.webUrl probably shouldn't accept hardcoded 'wiki/' param, check test/e2e/extra.e2e.test.ts
+    this.webUrl = this.baseUrlDirector.buildURL('wiki/')


Yes, here the mwWikiPath value should be taken

Refactored this. Also, 'wiki/' value is default again.

…rector

…mplate and renderer builder

…sue with outputDirectory

…rams

VadimKovalenkoSNF · 2023-11-12T10:40:30Z

@kelson42 It looks like the root cause was not with treating wikiPath for the third-party wikis, but rather how mwActionApPathi (ex mwApiPath) is set - it should not contain the forward slash at the beginning, because we build API endpoint on top of baseUrl.href in Mediawiki which already contains forward slash at the end.

Check this JS snippet:

const url = 'https://en.wikipedia.org'
console.log(new URL(url).href) // "https://en.wikipedia.org/"

Forward slash at the end of href prohibits building the correct URL for wikis like https://minecraft.wiki/: it will be like https://minecraft.wiki//api.php?action=... which is wrong (note two forward slashes before api.php/). So in such cases, the receipt should be changed. Here is a minified version of article scraping when using changes from this PR:
npm start -- --adminEmail="contact@kiwix.org" --customZimDescription="Minecraft is a sandbox construction video game." --mwActionApiPath="api.php" --mwModulePath="/load.php" --mwUrl="https://minecraft.wiki/" --mwWikiPath="/" --articleList="Enchanting" --verbose

By using this command, the action API URL to get wiki site info will be correct:
https://minecraft.wiki/api.php?action=query&meta=siteinfo&format=json&formatversion=2&siprop=general%7Cnamespaces%7Cstatistics%7Cvariables%7Ccategory%7Cwikidesc

So I brought back the default 'wiki/' param for Mediawiki.webUrl. It is using only there and nowhere else.

VadimKovalenkoSNF mentioned this pull request Oct 31, 2023

[REGRESSION?] API end-point detection does not take care properly of command line argument #1935

Closed

VadimKovalenkoSNF force-pushed the I1935-rollback-mw-api-configs branch from 8bbdd42 to 9240879 Compare November 3, 2023 06:46

VadimKovalenkoSNF marked this pull request as ready for review November 3, 2023 09:36

VadimKovalenkoSNF requested a review from kelson42 November 3, 2023 09:36

VadimKovalenkoSNF changed the title ~~Refactor mwApiPath and mwWikiPath parameters~~ Refactor mwApiPath, mwRestApiPath, mwModulePath and mwWikiPath parameters + fix regression of test render template Nov 3, 2023

kelson42 requested changes Nov 3, 2023

View reviewed changes

src/mwoffliner.lib.ts Outdated Show resolved Hide resolved

VadimKovalenkoSNF mentioned this pull request Nov 5, 2023

Temporary disable VisualEditor capability check #1941

Merged

VadimKovalenkoSNF force-pushed the I1935-rollback-mw-api-configs branch from 69654c6 to aa979c1 Compare November 6, 2023 12:54

VadimKovalenkoSNF requested a review from kelson42 November 6, 2023 13:49

VadimKovalenkoSNF mentioned this pull request Nov 7, 2023

Update setBaseUrl() to handle URL directors #1929

Merged

VadimKovalenkoSNF force-pushed the I1935-rollback-mw-api-configs branch 2 times, most recently from 99842b0 to c2f2b87 Compare November 9, 2023 12:37

kelson42 requested changes Nov 10, 2023

View reviewed changes

VadimKovalenkoSNF added 9 commits November 10, 2023 17:26

Rollback mwActionApiPath to mwApiPath

2a152b5

Refactor buildURL() calculation to include mwWikiPath

1f2ed9f

Add tests for handling mwApiPath and mwWikiPath params in base url di…

c7c6615

…rector

Update modulePath handling in Mediawiki

95a5460

Adjust tests, comment out VE capabilities check, refactor e2e test te…

10d6e76

…mplate and renderer builder

Add optionalRenderesList param to the testAllRenders template, fix is…

5a27896

…sue with outputDirectory

Refactor rimraf invocation in e2e tests

c5fe022

Optimize renderer builder and temporary disable VE test for it

39de207

Fixed issue with unconditional MW params setters side effects

c4ef4c4

VadimKovalenkoSNF added 5 commits November 10, 2023 17:26

Rename mwApiPath to mwActionApiPath, add e2e tests for mw api path pa…

e6078f1

…rams

Temp remove VE from wikisource.e2e.test.ts render list

ebf85b5

Fix Mediawiki.wikiPath setter

9b64386

Make default mwWikiPath as empty string

8422cb0

Refactor mwWikiPath, bring it back as 'wiki/' default value

d3b0125

VadimKovalenkoSNF force-pushed the I1935-rollback-mw-api-configs branch from c2f2b87 to d3b0125 Compare November 10, 2023 16:00

kelson42 merged commit 99da4fd into main Nov 13, 2023
3 of 4 checks passed

kelson42 deleted the I1935-rollback-mw-api-configs branch November 13, 2023 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor mwApiPath (to mwActionApiPath), mwRestApiPath, mwModulePath and mwWikiPath parameters + fix regression of test render template #1939

Refactor mwApiPath (to mwActionApiPath), mwRestApiPath, mwModulePath and mwWikiPath parameters + fix regression of test render template #1939

VadimKovalenkoSNF commented Oct 30, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

kelson42 commented Nov 3, 2023

kelson42 left a comment

kelson42 Nov 3, 2023

VadimKovalenkoSNF Nov 6, 2023

kelson42 Nov 3, 2023

VadimKovalenkoSNF Nov 6, 2023

benoit74 commented Nov 7, 2023

VadimKovalenkoSNF commented Nov 8, 2023

kelson42 left a comment

VadimKovalenkoSNF commented Nov 10, 2023

kelson42 Nov 10, 2023

VadimKovalenkoSNF Nov 12, 2023

kelson42 Nov 10, 2023

VadimKovalenkoSNF Nov 12, 2023

kelson42 Nov 10, 2023

VadimKovalenkoSNF Nov 12, 2023

kelson42 Nov 10, 2023

VadimKovalenkoSNF Nov 12, 2023

VadimKovalenkoSNF commented Nov 12, 2023 •

edited

Loading

Refactor mwApiPath (to mwActionApiPath), mwRestApiPath, mwModulePath and mwWikiPath parameters + fix regression of test render template #1939

Refactor mwApiPath (to mwActionApiPath), mwRestApiPath, mwModulePath and mwWikiPath parameters + fix regression of test render template #1939

Conversation

VadimKovalenkoSNF commented Oct 30, 2023 • edited Loading

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

kelson42 commented Nov 3, 2023

kelson42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoit74 commented Nov 7, 2023

VadimKovalenkoSNF commented Nov 8, 2023

kelson42 left a comment

Choose a reason for hiding this comment

VadimKovalenkoSNF commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VadimKovalenkoSNF commented Nov 12, 2023 • edited Loading

VadimKovalenkoSNF commented Oct 30, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

VadimKovalenkoSNF commented Nov 12, 2023 •

edited

Loading