Add unit tests, e2e test to parser service #3040

TueeNguyen · 2022-02-24T16:34:22Z

Issue This PR Addresses

Part of #2111

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

Most tests are copied from test/, I fixed the some imports and added a few cases
e2e test is completed

Steps to test the PR

Remove redis cache rm -rf redis-data or else it'll take very long and the e2e test will timeout
pnpm install
pnpm services:start elasticsearch redis traefik posts
pnpm jest:e2e src/api/parser

HOWEVER
If you

pnpm services:start elasticsearch posts redis traefik
pnpm jest:e2e src/api/parser

It'll error out because the change in #3248 make all e2e tests to wait for Postgre. So you should pnpm services:start and run the test.

This could be annoying when we have more e2e tests and want to run only a specific e2e test in development mode

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

gitpod-io · 2022-02-24T16:34:24Z

TueeNguyen · 2022-02-24T16:35:11Z

How can we replace indexer with ES mock @rclee91?

JerryHue · 2022-03-01T22:47:12Z

src/api/parser/jest.config.js

@@ -3,6 +3,7 @@ const baseConfig = require('../../../jest.config.base');
 module.exports = {
  ...baseConfig,
  rootDir: '../../..',
+  setupFiles: ['<rootDir>/src/api/parser/jest.setup.js', '<rootDir>/test/jest.setup.js'],


Is there a reason to include the setup file from /test? Don't we want to only include the test that focus on the parser alone?

@rclee91 what do you think?

There is a node-fetch mock In the /test/jest.setup.js plus some other configs.
Parser currently uses node-fetch from Satellite, but it's also just the regular node-fetch
But true, if the backend will end up getting ripped one day, we should also move the setup stuff from /test to the the setup files in Parser tests.

src/api/parser/test/feed-queue.test.js

src/api/parser/test/wiki-feed-parser.test.js

src/api/parser/test/url-parser.test.js

src/api/parser/test/remove-empty-paragraphs.test.js

src/api/parser/test/remove-empty-anchor.test.js

JerryHue · 2022-03-02T19:24:53Z

src/api/parser/test/remove-empty-anchor.test.js

+  test('Should remove <a></a> ', () => {
+    const firstHTMLBefore = '<div><a></a></div>';
+    const firstHTMLAfter = removeNoContentAnchor(firstHTMLBefore);
+    const firstHTMLExpected = '<div></div>';
+
+    const secondHTMLBefore = '<div><a></a> This is the content of the outer div</div>';
+    const secondHTMLAfter = removeNoContentAnchor(secondHTMLBefore);
+    const secondHTMLExpected = '<div> This is the content of the outer div</div>';
+
+    expect(firstHTMLAfter).toEqual(firstHTMLExpected);
+    expect(secondHTMLAfter).toEqual(secondHTMLExpected);
+  });


Could we try to separate these two test cases, instead of having them as a single case?
We could separate them so that one is described as Should remove <a></a> when outer div is empty and the other as Should remove <a></a> when outer div is not empty.

src/api/parser/test/feed.test.js

TueeNguyen · 2022-03-03T20:58:55Z

package.json

@@ -18,7 +18,7 @@
    "prettier-check": "prettier --check \"./**/*.{md,jsx,json,html,css,js,yml}\"",
    "test": "pnpm jest",
    "jest": "jest -c jest.config.js --",
-    "jest:e2e": "jest -c jest.config.e2e.js --",
+    "jest:e2e": "jest -c jest.config.e2e.js --forceExit",


I added --forceExit because after e2e tests are finished, there are a few open Redis connections and open fetch or logger( this I'm not sure). Jest will be hung and not exit.

telescope/src/api/parser/src/lib/queue.js

Lines 9 to 10 in 95dde6c

const client = Redis();

const subscriber = Redis();

humphd

...And just as I click "submit review" I remember that our CI workflows won't run what's submitted in PRs, only what we do on master.

Can you break the change to .github/workflows/e2e-tests-ci.yml out into a separate PR we can land right away, and then you can rebase on it to run these with ES.

TueeNguyen · 2022-03-23T14:29:26Z

@humphd, I reverted the CI change.

humphd · 2022-03-23T14:34:21Z

Please do a PR for the CI change on its own, or these will never pass.

TueeNguyen · 2022-03-23T22:40:46Z

@humphd, it passed all checks 🎆

humphd

We should look into why Playwright takes so long to setup in CI. They have a Docker image we can use, which might be faster.

This is good to go I think!

TueeNguyen · 2022-03-24T01:31:44Z

For anyone who's testing this out. If you

pnpm services:start elasticsearch posts redis traefik
pnpm jest:e2e src/api/parser

It'll error out because the change in #3248 make all e2e tests to wait for Postgre. So you should start all services and run the test.

This could be annoying when we have more e2e tests and want to run only a specific e2e test in development mode

HyperTHD

Just one last thing and this is good

HyperTHD · 2022-03-24T01:33:13Z

src/api/parser/test/sanitize-html.test.js

+  });
+
+  test('twitch.tv embedded content should not be removed', () => {
+    const data = sanitizeHTML('<iframe src="https://www.twitch.tv/0pensrc"></iframe>');


Embedded twitch content doesn't come from here, should be from player.twitch.tv
Thought I added the test for this here though, strange it's not here

Thanks for pointing it out, maybe it got messed up when rebasing

Kevan-Y

Works tested on local.

TueeNguyen · 2022-03-24T13:27:07Z

@HyperTHD, I think it's good to go but I probably need your last review on the sanitize file

- Add containers needed for parser e2e in workflow file - Most of them are copied over from ./test - using ES-mock for parser tests - Destructure index.js -> parser.js and index.js - Add e2e test - Add some fixes to unit tests - Remove setIntervalAsync to use setInterval from 'timers' - Add '--forceExit' to exit - Moved backend test setup to Parser test setup - Cleaned up url-parser tests - Refactored wiki-feed-parser tests to use mocked fetch from Satellite Co-authored: @rclee91

HyperTHD

LGTM @TueeNguyen
Just make sure you rebase prior to merging

- Add containers needed for parser e2e in workflow file - Most of them are copied over from ./test - using ES-mock for parser tests - Destructure index.js -> parser.js and index.js - Add e2e test - Add some fixes to unit tests - Remove setIntervalAsync to use setInterval from 'timers' - Add '--forceExit' to exit - Moved backend test setup to Parser test setup - Cleaned up url-parser tests - Refactored wiki-feed-parser tests to use mocked fetch from Satellite Co-authored: @rclee91

vercel bot deployed to Preview February 24, 2022 16:34 View deployment

TueeNguyen force-pushed the parser-tests branch from fb7d09a to 6cb086d Compare February 24, 2022 16:36

vercel bot deployed to Preview February 24, 2022 16:36 View deployment

aserputov added area: microservices type: test Creation and development of test labels Feb 24, 2022

aserputov linked an issue Feb 24, 2022 that may be closed by this pull request

Write tests and enable Parser service #2111

Closed

TueeNguyen changed the title ~~Add some tests to parser (most of them are ported from test/), add initial draft of e2e test.~~ Add some tests to parser, add initial draft of e2e test. Feb 25, 2022

TueeNguyen self-assigned this Feb 25, 2022

TueeNguyen added this to the 2.8 Release milestone Feb 25, 2022

JerryHue added the review: hard label Mar 1, 2022

JerryHue reviewed Mar 2, 2022

View reviewed changes

vercel bot deployed to Preview March 2, 2022 20:36 View deployment

TueeNguyen force-pushed the parser-tests branch from da6ffcc to 9e966c5 Compare March 2, 2022 20:42

vercel bot deployed to Preview March 2, 2022 20:43 View deployment

vercel bot deployed to Preview March 3, 2022 05:35 View deployment

vercel bot deployed to Preview March 3, 2022 05:37 View deployment

vercel bot deployed to Preview March 3, 2022 18:40 View deployment

TueeNguyen force-pushed the parser-tests branch from 5c941bd to 73156ba Compare March 3, 2022 18:45

vercel bot deployed to Preview March 3, 2022 18:45 View deployment

TueeNguyen force-pushed the parser-tests branch from 73156ba to 6d28116 Compare March 3, 2022 18:53

vercel bot deployed to Preview March 3, 2022 18:53 View deployment

TueeNguyen force-pushed the parser-tests branch from 6d28116 to 630208e Compare March 3, 2022 20:06

vercel bot deployed to Preview March 3, 2022 20:06 View deployment

TueeNguyen force-pushed the parser-tests branch from 630208e to f042c15 Compare March 3, 2022 20:53

vercel bot deployed to Preview March 3, 2022 20:53 View deployment

TueeNguyen commented Mar 3, 2022

View reviewed changes

TueeNguyen marked this pull request as ready for review March 6, 2022 00:17

TueeNguyen force-pushed the parser-tests branch from f042c15 to 7d85a26 Compare March 6, 2022 00:22

humphd reviewed Mar 23, 2022

View reviewed changes

TueeNguyen force-pushed the parser-tests branch from ac268b7 to 7872969 Compare March 23, 2022 14:20

vercel bot deployed to Preview March 23, 2022 14:20 View deployment

TueeNguyen requested a review from humphd March 23, 2022 14:31

TueeNguyen force-pushed the parser-tests branch from 7872969 to fce954d Compare March 23, 2022 15:32

vercel bot deployed to Preview March 23, 2022 16:25 View deployment

TueeNguyen force-pushed the parser-tests branch from fce954d to 950c662 Compare March 23, 2022 21:22

vercel bot deployed to Preview March 23, 2022 21:22 View deployment

humphd previously approved these changes Mar 23, 2022

View reviewed changes

HyperTHD requested changes Mar 24, 2022

View reviewed changes

TueeNguyen dismissed humphd’s stale review via a91eb10 March 24, 2022 01:39

TueeNguyen force-pushed the parser-tests branch from 950c662 to a91eb10 Compare March 24, 2022 01:39

vercel bot deployed to Preview March 24, 2022 01:39 View deployment

TueeNguyen mentioned this pull request Mar 24, 2022

Dockerize Parser service #3302

Merged

8 tasks

TueeNguyen force-pushed the parser-tests branch from a91eb10 to 2862696 Compare March 24, 2022 01:51

vercel bot deployed to Preview March 24, 2022 01:51 View deployment

Kevan-Y approved these changes Mar 24, 2022

View reviewed changes

joelazwar approved these changes Mar 24, 2022

View reviewed changes

AmasiaNalbandian force-pushed the parser-tests branch from 2862696 to 69f837e Compare March 24, 2022 13:27

vercel bot deployed to Preview March 24, 2022 13:27 View deployment

HyperTHD self-requested a review March 24, 2022 15:25

HyperTHD approved these changes Mar 24, 2022

View reviewed changes

AmasiaNalbandian merged commit b412cd0 into Seneca-CDOT:master Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests, e2e test to parser service #3040

Add unit tests, e2e test to parser service #3040

TueeNguyen commented Feb 24, 2022 •

edited

gitpod-io bot commented Feb 24, 2022

TueeNguyen commented Feb 24, 2022

JerryHue Mar 1, 2022

TueeNguyen Mar 3, 2022

RC-Lee Mar 8, 2022

JerryHue Mar 2, 2022

TueeNguyen Mar 3, 2022

humphd left a comment

TueeNguyen commented Mar 23, 2022

humphd commented Mar 23, 2022

TueeNguyen commented Mar 23, 2022

humphd left a comment

TueeNguyen commented Mar 24, 2022

HyperTHD left a comment

HyperTHD Mar 24, 2022

TueeNguyen Mar 24, 2022

Kevan-Y left a comment •

edited

TueeNguyen commented Mar 24, 2022

HyperTHD left a comment

Add unit tests, e2e test to parser service #3040

Add unit tests, e2e test to parser service #3040

Conversation

TueeNguyen commented Feb 24, 2022 • edited

Issue This PR Addresses

Type of Change

Description

Steps to test the PR

Checklist

gitpod-io bot commented Feb 24, 2022

TueeNguyen commented Feb 24, 2022

JerryHue Mar 1, 2022

Choose a reason for hiding this comment

TueeNguyen Mar 3, 2022

Choose a reason for hiding this comment

RC-Lee Mar 8, 2022

Choose a reason for hiding this comment

JerryHue Mar 2, 2022

Choose a reason for hiding this comment

TueeNguyen Mar 3, 2022

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

TueeNguyen commented Mar 23, 2022

humphd commented Mar 23, 2022

TueeNguyen commented Mar 23, 2022

humphd left a comment

Choose a reason for hiding this comment

TueeNguyen commented Mar 24, 2022

HyperTHD left a comment

Choose a reason for hiding this comment

HyperTHD Mar 24, 2022

Choose a reason for hiding this comment

TueeNguyen Mar 24, 2022

Choose a reason for hiding this comment

Kevan-Y left a comment • edited

Choose a reason for hiding this comment

TueeNguyen commented Mar 24, 2022

HyperTHD left a comment

Choose a reason for hiding this comment

TueeNguyen commented Feb 24, 2022 •

edited

Kevan-Y left a comment •

edited