refactor: working necromancy tests #1101

whizzzkid · 2022-10-22T17:55:35Z

Enhances #1054

In this PR:

I am replacing require with import/export
Upgrading even more packages.
Fixing webExt missing config
Replacing nyc with c8

whizzzkid

explainers

package.json

whizzzkid · 2022-10-26T15:12:37Z

test/functional/lib/ipfs-companion.test.js

+global.chrome = browser
+global.navigator = {
+  clipboard: {
+    writeText: () => {}


this was something extra I had to add to prevent dependency from failing.

which dependency? Can we add a comment to the code to call it out? Do we need that dependency?

whizzzkid

more explainers.

whizzzkid · 2022-10-26T22:11:21Z

test/functional/lib/ipfs-companion.test.js

+}
+// We need to do this because global is not mapped otherwise, we need to stub browser and chrome runtime
+// so that the webextension-polyfill does not complain about the test runner not being a browser instance.
+const init = async () => (await import('../../../add-on/src/lib/ipfs-companion.js')).default()


this was really the problem I had to solve because how node's module resolution works. stubbing global.browser was not possible when import for a module is called.

However doing it this way allows us to pass in stubbed references.

whizzzkid · 2022-10-26T22:12:06Z

package.json

@@ -37,7 +38,7 @@
    "watch:js": "run-p watch:js:*",
    "watch:js:webpack": "webpack --watch --mode development --devtool inline-source-map  --config ./webpack.config.js",
    "test": "run-s test:*",
-    "test:functional": " nyc --reporter=lcov --reporter=text mocha --timeout 15000 --require ignore-styles \"test/functional/**/*.test.js\"",
+    "test:functional": "c8 mocha --timeout 5000 --require ignore-styles \"test/functional/**/*.test.js\"",


c8 is a drop-in replacement for nyc. nyc fails with ESM on Node > 16.12. Src:

todo: figure out how to instrument .mjs files istanbuljs/nyc#659

Nyc on ESM Node.js 13 (no babel) istanbuljs/nyc#1287

https://stackoverflow.com/a/68095739

whizzzkid · 2022-10-26T22:12:47Z

package.json

+  "c8": {
+    "all": true,
+    "reporter": [
+      "lcov",
+      "text"
+    ],
+    "include": [
+      "add-on/src/**"
+    ]
+  },


moving configs like so instead of flags

whizzzkid · 2022-10-26T22:14:59Z

add-on/src/lib/precache.js

+// import  CID  from 'cids'

-// const Tar = require('it-tar')
-// const pipe = require('it-pipe')
-// const all = require('it-all')
-// const concat = require('it-concat')
+// import  Tar  from 'it-tar'
+// import  pipe  from 'it-pipe'
+// import  all  from 'it-all'
+// import  concat  from 'it-concat'


do we need these? this got replaced as part of the regex match.

Looks like they are all for "downloading CID from public gateway". It's commented out code so I would say it's good to be removed. It's in git history if you ever want to reference it. But @lidel may have more insight

Yes, this is legacy code that needs to be replaced – filled #1102 so it is not blocking necromancy PRs ;)

whizzzkid · 2022-10-26T22:15:33Z

add-on/src/pages/proxy-access-dialog/store.js

@@ -1,6 +1,6 @@
 'use strict'

-function createProxyAccessDialogStore (i18n, runtime) {
+export default function createProxyAccessDialogStore (_i18n, runtime) {


just to make the editor happy, we are not using this.

whizzzkid · 2022-10-26T22:16:19Z

add-on/src/lib/options.js

@@ -1,9 +1,9 @@
 'use strict'

-const isIP = require('is-ip')
-const isFQDN = require('is-fqdn')
+import { isIPv4, isIPv6 } from 'is-ip'


isIP@4 has this breaking change.

whizzzkid · 2022-11-14T06:32:29Z

@lidel bringing this up again for your consideration! ❤️

meandavejustice

👍 Left a couple comments since I was here

meandavejustice · 2022-11-15T21:38:05Z

add-on/src/lib/precache.js

+// import  CID  from 'cids'

-// const Tar = require('it-tar')
-// const pipe = require('it-pipe')
-// const all = require('it-all')
-// const concat = require('it-concat')
+// import  Tar  from 'it-tar'
+// import  pipe  from 'it-pipe'
+// import  all  from 'it-all'
+// import  concat  from 'it-concat'


Looks like they are all for "downloading CID from public gateway". It's commented out code so I would say it's good to be removed. It's in git history if you ever want to reference it. But @lidel may have more insight

meandavejustice · 2022-11-15T21:43:06Z

test/functional/lib/dnslink.test.js

+import { expect } from 'chai'
+import { URL } from 'url'
+import sinon from 'sinon'
+import createDnslinkResolver from '../../../add-on/src/lib/dnslink.js'


NIT: .js shouldn't be necessary for these imports.

we should standardize on esm imports and the .js extension is part of the filename. We should be consistent even if there is tooling that does "magic" for us. Open to hearing your thoughts @meandavejustice

package.json

lidel

I'll try to block some time this week to review this properly (there are some manual tests around Brave onboarding which I need to go over + document in ./docs).

For now, dropped quick comments above.

SgtPooki · 2022-11-17T20:08:43Z

add-on/src/lib/constants.js

-exports.tickMs = 250 // no CPU spike, but still responsive enough
+export const welcomePage = '/dist/landing-pages/welcome/index.html'
+export const optionsPage = '/dist/options/options.html'
+export const tickMs = 250 // no CPU spike, but still responsive enough


I would love a JSdoc comment on this to explain its intention. I can guess, but it's better if we don't force others to.

SgtPooki · 2022-11-17T20:11:49Z

add-on/src/lib/dnslink.js

+import Pqueue from 'p-queue'
+
+import debug from 'debug'
+import IsIpfs from 'is-ipfs'
+import LRU from 'lru-cache'
+import { offlinePeerCount } from './state.js'
+import { ipfsContentPath, sameGateway, pathAtHttpGateway } from './ipfs-path.js'


nit: spacing - I personally prefer organizing imports in four (potentially 5 groups depending on how large your 1st party import sections are):

stdlib imports;

3p imports;

2p imports (internal to company but not to team)

1p imports (internal to team)

local/relative imports (internal to project)

eslint-plugin-imports allows you to automate this

SgtPooki · 2022-11-17T20:13:21Z

add-on/src/lib/ipfs-client/external.js

+export async function destroy (browser) {
  log('destroy')
 }


remove / delete?

SgtPooki · 2022-11-17T20:18:02Z

add-on/src/popup/browser-action/gateway-status.js

  gatewayAddress,
  gatewayVersion,
-  swarmPeers,
-  ipfsNodeType
+  ipfsApiUrl,
+  ipfsNodeType,
+  swarmPeers


if you're alphabetizing things like this manually, stop and wait for eslint automation. I prefer organized keys and imports as well, but they will quickly get out of date without a 'fixable' eslint rule.

package.json

SgtPooki · 2022-11-17T20:24:31Z

test/functional/lib/dnslink.test.js

+import { expect } from 'chai'
+import { URL } from 'url'
+import sinon from 'sinon'
+import createDnslinkResolver from '../../../add-on/src/lib/dnslink.js'


we should standardize on esm imports and the .js extension is part of the filename. We should be consistent even if there is tooling that does "magic" for us. Open to hearing your thoughts @meandavejustice

SgtPooki · 2022-11-17T20:26:31Z

test/functional/lib/ipfs-companion.test.js

+global.chrome = browser
+global.navigator = {
+  clipboard: {
+    writeText: () => {}


which dependency? Can we add a comment to the code to call it out? Do we need that dependency?

SgtPooki · 2022-11-17T20:27:40Z

test/functional/lib/ipfs-request-dnslink.test.js

@@ -27,6 +27,7 @@ describe('modifyRequest processing of DNSLinks', function () {
  before(function () {
    global.URL = URL
    global.browser = browser
+    browser.runtime.id = 'testid'


We're using the same id for browser.runtime.id in multiple tests. What is browser.runtime.id used for, and would it aid in test debugging to provide a different id?

Can we set the id to this.testName or similar?

test/helpers/acl.js

#1101 (comment)

lidel

Thank you @whizzzkid.
In effort to unblock general Companion work I'm merging, let's continue in #1054 and/or separate PRs.

- removed yarn and all the hackery around transitive dependency overrides - bumped all dependencies - switched to ipfs-core for less deps - removed unused deps - firefox manifest fix to pass latest webext lint - removed remaining window.ipfs code that was pulling dead dependencies (ipfs/in-web-browsers#172) - fixed tests (#1101) - and much more Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

whizzzkid added 3 commits October 21, 2022 02:04

Adding esm config

db64576

Moving to import/export - it builds

2f5f41d

Fixed tests

b00a823

whizzzkid mentioned this pull request Oct 22, 2022

refactor: necromancy to restore MV2 build #1054

Merged

19 tasks

whizzzkid added 4 commits October 26, 2022 15:09

Fixing Test

9cf9c79

typo

b5d28ce

fixing breaking is-ip changes

442ac60

Fixing coverage reporting

66d3594

whizzzkid commented Oct 26, 2022

View reviewed changes

adding missing webExt config

4f3cab1

whizzzkid force-pushed the fix/refactor/necromancy branch from 47e50b6 to 4f3cab1 Compare October 26, 2022 15:20

whizzzkid added 3 commits October 26, 2022 16:41

Fixing test

d6c84df

Passing abort-controller as dep

82b7089

Fixing Lint

9a70e80

whizzzkid marked this pull request as ready for review October 26, 2022 16:26

whizzzkid requested review from lidel, meandavejustice and a team as code owners October 26, 2022 16:26

whizzzkid commented Oct 26, 2022

View reviewed changes

meandavejustice approved these changes Nov 15, 2022

View reviewed changes

lidel reviewed Nov 16, 2022

View reviewed changes

lidel changed the title ~~Fix refactor/necromancy~~ refactor: working necromancy tests Nov 16, 2022

SgtPooki requested changes Nov 17, 2022

View reviewed changes

lidel added 3 commits November 21, 2022 19:18

chore: remove window.ipfs ACL tests

9184147

chore: bump terser deps

3ff5d3d

chore: pin all dependency versions

c8fc841

#1101 (comment)

lidel approved these changes Nov 21, 2022

View reviewed changes

lidel merged commit a848969 into refactor/necromancy Nov 21, 2022

lidel deleted the fix/refactor/necromancy branch November 21, 2022 18:54

This was referenced Nov 22, 2022

Switch ipfs-webui preload to CAR fetch from gateway #1102

Closed

fix(ci): hanging tests #1106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: working necromancy tests #1101

refactor: working necromancy tests #1101

whizzzkid commented Oct 22, 2022 •

edited

Loading

whizzzkid left a comment

whizzzkid Oct 26, 2022

SgtPooki Nov 17, 2022

whizzzkid left a comment

whizzzkid Oct 26, 2022

whizzzkid Oct 26, 2022

whizzzkid Oct 26, 2022

whizzzkid Oct 26, 2022

meandavejustice Nov 15, 2022

lidel Nov 16, 2022

whizzzkid Oct 26, 2022

whizzzkid Oct 26, 2022

whizzzkid commented Nov 14, 2022

meandavejustice left a comment

meandavejustice Nov 15, 2022

meandavejustice Nov 15, 2022

SgtPooki Nov 17, 2022

lidel left a comment •

edited

Loading

SgtPooki Nov 17, 2022

SgtPooki Nov 17, 2022

SgtPooki Nov 17, 2022

SgtPooki Nov 17, 2022

SgtPooki Nov 17, 2022

SgtPooki Nov 17, 2022

SgtPooki Nov 17, 2022

lidel left a comment •

edited

Loading

refactor: working necromancy tests #1101

refactor: working necromancy tests #1101

Conversation

whizzzkid commented Oct 22, 2022 • edited Loading

whizzzkid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid 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

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

whizzzkid commented Nov 14, 2022

meandavejustice 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

lidel left a comment • edited Loading

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

lidel left a comment • edited Loading

Choose a reason for hiding this comment

whizzzkid commented Oct 22, 2022 •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading