phpunit tests converted to mocha/chai #140

abaevbog · 2023-05-19T03:46:02Z

v1 and v2 tests that do not require s3 integrations.

dstillman · 2023-05-26T11:21:48Z

tests/remote_js/data/sync1download.xml

sync1download.xml and sync2upload.xml can be removed. These were from the old sync system.

abaevbog · 2023-05-25T15:05:46Z

tests/remote_js/test/3/fileTest.js

+				json.prefix + fileContents + json.suffix,
+				{ "Content-Type": json.contentType }
+			);
+			Helpers.assert201(response);


@dstillman
This call fails with a complaint that the size does not match what was proposed.
The general issue is that there seems to be a different number of bytes in a file vs its stringified representation that is put into the body of post requests.
I worked around this by using the bytes of an already stringified buffer before getting authorization and then doing the upload - see return values from generateZip function as an example.
Here, though, md5 and zipMD5 are hardcoded on the actual dataserver so when I tried to use bytes of stringified file content, I got an error during authorization "Specified file size incorrect for known file"

abaevbog · 2023-05-26T11:46:06Z

tests/remote_js/test/2/fileTest.js

+		assert.equal(contentType, json.contentType);
+		assert.equal(charset, json.charset);
+		const updated = Helpers.xpathEval(xml, '/atom:entry/atom:updated');
+		assert.notEqual(serverDateModified, updated);


This line fails. In phpunit test, this condition "should" always fail as well.

There seems to be an error with xpath in phpunit on this line: $serverDateModified = array_get_first($xml->xpath('/atom:entry/atom:updated'));. It makes serverDateModified always null, so on this line, $this->assertNotEquals($serverDateModified, array_get_first($xml->xpath('/atom:entry/atom:updated'))); always passes because one of the values is null.

Fixing the xpath in phpunit for //atom:entry/atom:updated makes serverDateModified not null but then it is the same as updated value fetched from the xml, so the assertion fails.

So this assertion fails, just like it should in phpunit. I notice this condition was removed in V3. Should we remove it here as well?

dstillman

Still going through this, but some initial comments

dstillman · 2023-05-30T04:53:52Z

tests/remote_js/api3.js

+const fs = require("fs");
+const wgxpath = require('wgxpath');
+
+class API3 extends API2 {


I don't think we want to extend here. v3 is the default version, and we could deprecate older versions at any time (we haven't in a decade or more, but we could…), so it doesn't make sense to make this dependent on an older version. If different versions need different helpers, they should just be separate files with some duplicate code, and we ideally never touch the older ones again.

Right, I did the extension to avoid duplicate code at first but since it is not an issue, I made independent api2/3 and helpers2/3 that have some functions overlapping

dstillman · 2023-05-30T05:07:00Z

tests/remote_js/package.json

+  },
+  "scripts": {
+    "test": "mocha \"test/**/*.js\"",
+    "test_file": "mocha \"test/3/publicationTest.js\""


I assume this was just for development?

Right, shouldn't have committed it in the first place. I'll get rid of this

dstillman · 2023-05-30T09:59:40Z

tests/remote_js/config.json

@@ -0,0 +1,35 @@
+{
+    "verbose": 1,
+    "syncURLPrefix": "http://dataserver/",


Don't need this

dstillman · 2023-05-30T10:59:00Z

tests/remote_js/config.js

+config = JSON.parse(data);
+config.timeout = 60000;
+config.numOwnedGroups = 3;
+config.numPublicGroups = 2;


What's the logic for why these are in here and not in config.json?

In any case, we should use the npm config package and move everything into a default.json5 file in config (like translation-server).

And then we could either do a find-and-replace to switch to the recommended access method or we could just stick to direct property access. get() would let us set values (e.g., rootUsername/rootPassword) to undefined in default.json to force them to be populated in a local.json, but since these are tests anyway, they'll just fail if something isn't configured properly, so maybe it's not worth the extra noise.

Nothing specific here. I think the config.ini used in phpunit did not have those fields, and they were set in some of the other php files. I was converting files one by one, so it just turned out that way.
Anyways, after my cleanup, I removed it all and setup the node-config with default.json5

dstillman · 2023-05-30T10:59:16Z

tests/remote_js/config.json

+    "awsAccessKey": "",
+    "awsSecretKey": "",
+    "filesystemStorage": 1,
+    "syncVersion": 9,


Don't need this

dstillman · 2023-05-30T11:01:07Z

tests/remote_js/api3.js

+
+	static async resetSchemaVersion() {
+		const schema = JSON.parse(fs.readFileSync("../../htdocs/zotero-schema/schema.json"));
+		this.schemaVersion = schema;


This appears to be storing the entire file, not the schema version.

dstillman · 2023-05-30T11:19:40Z

tests/remote_js/helpers3.js

+const crypto = require('crypto');
+const fs = require('fs');
+
+class Helpers3 extends Helpers {


Same here as API3.

dstillman · 2023-05-30T11:25:35Z

tests/remote_js/package.json

+{
+  "dependencies": {
+    "@aws-sdk/client-s3": "^3.338.0",
+    "axios": "^1.4.0",


Axios is gone, right?

Yes, sorry. I just didn't push package.json until just now

dstillman · 2023-05-30T20:56:51Z

tests/remote_js/httpHandler.js

+				if (error.name === 'FetchError') {
+					console.log('Request aborted. Wait for 2 seconds and retry...');
+					await new Promise(r => setTimeout(r, 2000));
+					tried += 1;


What was the deal with this? Clients should obviously retry requests automatically, but that's not really something we want in our tests, since it shouldn't be necessary and could hide actual problems. Can we just fix whatever the server problem is that led to this?

So I've been having the issue with the socket hanging up every now and then when I run test files. Happened randomly on different tests.
This issue never happened with php-unit, so I wasn't sure what to think. I added this wrapper to be able to continue working on tests without getting stuck on figuring out the dataserver configuration.

Now that the tests are virtually done, I will dig into apache config on the zotero-dataserver. It must be something from there

dstillman · 2023-05-30T21:05:46Z

tests/remote_js/test/shared.js

+	},
+	API1WrapUp: async () => {
+		await API.userClear(config.userID);
+	},


Let's use Before and After like Mocha, SetUp and TearDown like PHPUnit, Start/Stop, Start/Shutdown, or something else consistent and distinct.

(Setup is a noun, so would need to be SetUp to be consistent, but SetUp and WrapUp are a bit too similar.)

I renamed them for API#Before and API#After

…eneral test

dstillman · 2023-05-31T03:52:51Z

tests/remote_js/httpHandler.js

-		if (!success) {
-			throw new Error(`${method} to ${url} did not succeed after ${attempts} attempts.`);
-		}
+		await new Promise(resolve => setTimeout(resolve, 0));


You should add a comment with a link to the GitHub issue so we can track that bug.

I just meant to comment the line so we know why this is here, not to add a comment on GitHub.

Got it! My bad

I mean literally add a comment to the source code above this line and commit it. Nothing to do with our GitHub. There just shouldn’t be random unexplained lines in the source code that will stay there forever because we don’t know why they’re there.

Understood. Sorry about that

abaevbog · 2023-06-01T04:18:54Z

tests/remote_js/httpHandler.js

+// due to bug in node-fetch: https://github.com/node-fetch/node-fetch/issues/1735
+const httpAgent = new http.Agent({ keepAlive: true });
+const httpsAgent = new https.Agent({ keepAlive: true });
+const agentSelector = function (_parsedURL) {


Despite adding a tiny delay before each API call, the socket hanging happened again tonight as I was running tests. I added http agents with keepalive=true as another workaround suggested in the issue. Seems to be helping but I will keep track on this. Thinking to try out axios just so see if it would help

abaevbog · 2023-06-01T15:21:52Z

I was wrong to blame node-fetch bug for the socket hanging issue. I re-wrote httpHandler with node:http instead of node-fetch, and the same issue remained. Then, I finally managed to reproduce the issue with phpunit - similar thing, just does not pop up as often. So @dstillman you are right, it must be some backend config of zotero-server that is the real culprit.

…s from httpHandler

For zotero#152

test for zotero#154

Convert phpunit test updates to mocha Skipping tests depending on not merged PRs Fix to failing file test due to wrong content size

New converted test.

phpunit test v1 and v2 converted to mocha/chai

cfc8228

abaevbog requested a review from dstillman May 19, 2023 03:46

abaevbog self-assigned this May 19, 2023

abaevbog added 9 commits May 22, 2023 09:41

helper functions for api v3, annotation, atom and creator tests for v3

d58206e

v3 collection, group, fullText and note test

0244f16

v3 object test

f4dfc10

leftover object tests, mappings, params tests

b30737f

previously skipped tests requiring citation or translation server

aae33b1

file tests v2 and half of v3

878afef

the rest of file tests

e8449db

publication tests

8b7804b

minor adjustments to prevent random 500 errors

bf083e1

dstillman reviewed May 26, 2023

View reviewed changes

abaevbog commented May 26, 2023

View reviewed changes

abaevbog added 3 commits May 26, 2023 16:38

more v3 tests

6af7367

removing work dir, removed unneeded data/ files

9e746e0

notification, tag and version tests

339d923

dstillman reviewed May 30, 2023

View reviewed changes

abaevbog added 4 commits May 30, 2023 13:07

v3 itemTest

2f5ccd0

some cleanup, proper retry logic when socket hangs

755e133

using node-config

319ce22

removed inheritance from between api files and helpers

1ee44b1

dstillman reviewed May 30, 2023

View reviewed changes

abaevbog added 3 commits May 30, 2023 17:18

package.json

41d5981

changed 'Setup' for 'Before' and 'Wrapup' for 'After' in shared.js, g…

4d7632c

…eneral test

removing retries, using 0-second delay instead

e150cd1

abaevbog changed the title ~~phpunit test v1 and v2 converted to mocha/chai~~ phpunit tests converted to mocha/chai May 31, 2023

dstillman reviewed May 31, 2023

View reviewed changes

minor cleanup, added missing comments, some tweaking with socker hanging

ae270e5

abaevbog commented Jun 1, 2023

View reviewed changes

zotero deleted a comment from abaevbog Jun 1, 2023

abaevbog added 3 commits June 2, 2023 17:20

using helper functions for statusForObject + removed unnecessary part…

0d653ed

…s from httpHandler

initial pdf text extraction setup

5e21351

pdf text extraction test

8e0936b

abaevbog force-pushed the mocha-chai-tests branch from e719c89 to 8e0936b Compare June 9, 2023 14:57

abaevbog added 9 commits June 12, 2023 12:32

full-text tests with indexing function invoked locally

1976445

items/unfiled and items/unfiled/tags tests

d8f3427

For zotero#152

?sort=editBy for group libraries

2e7a853

test for zotero#154

test to rename tag for PR zotero#150

d4465f2

test for literal || escaping for tags PR zotero#143

e7518c2

new annotation types test

c5b1a93

minor cleanup, skip tests for PRs, missing tests

640caf4

Convert phpunit test updates to mocha Skipping tests depending on not merged PRs Fix to failing file test due to wrong content size

skip annotation tags test for collections

8253f4d

should_add_zero_integer_value_for_lastPageIndex

b2b562a

New converted test.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

phpunit tests converted to mocha/chai #140

phpunit tests converted to mocha/chai #140

abaevbog commented May 19, 2023

dstillman May 26, 2023

abaevbog May 25, 2023

abaevbog May 26, 2023 •

edited

dstillman left a comment

dstillman May 30, 2023

abaevbog May 30, 2023

dstillman May 30, 2023

abaevbog May 30, 2023

dstillman May 30, 2023

dstillman May 30, 2023

abaevbog May 30, 2023

dstillman May 30, 2023

dstillman May 30, 2023

dstillman May 30, 2023

dstillman May 30, 2023

abaevbog May 30, 2023

dstillman May 30, 2023

abaevbog May 30, 2023

dstillman May 30, 2023

abaevbog May 30, 2023

dstillman May 31, 2023 •

edited

dstillman May 31, 2023

abaevbog May 31, 2023

dstillman May 31, 2023

abaevbog May 31, 2023

abaevbog Jun 1, 2023

abaevbog commented Jun 1, 2023

phpunit tests converted to mocha/chai #140

Are you sure you want to change the base?

phpunit tests converted to mocha/chai #140

Conversation

abaevbog commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abaevbog May 26, 2023 • edited

Choose a reason for hiding this comment

dstillman 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

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

dstillman May 31, 2023 • edited

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

abaevbog commented Jun 1, 2023

abaevbog May 26, 2023 •

edited

dstillman May 31, 2023 •

edited