tchanders opened https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/179

output-sql.js: Fix tunnels arrays comparison

kharlan closed https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/181

output-sql.js: Fix tunnels arrays comparison

kharlan merged https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/179

output-sql.js: Fix tunnels arrays comparison

Change 974497 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[operations/deployment-charts@master] ipoid: Bump version

https://gerrit.wikimedia.org/r/974497

Change 974497 merged by jenkins-bot:

[operations/deployment-charts@master] ipoid: Bump version

https://gerrit.wikimedia.org/r/974497

I can no longer reproduce the error from the description.

In my previous testing, I had missed this bug because I hadn't appreciated that the diff algorithm compares one field at a time and returns early as soon as it finds one that is different. Fields that are compared earlier in the code were better tested than fields that are compared later. Therefore, while testing this, I made sure that I only changed one field at a time for each IP. I also generated data randomly rather than relying on real data,

It occurred to me that it might be easier to test this at the unit level, at least from the perspective of cognitive complexity of generating test data. I experimented with unit testing the individual functions in output-sql.js using random data generators. It is certainly possible to trigger other exceptions (some I have raised as bugs), especially if you allow the random generator to produce any kind of data it wants.

Other exceptions
I haven't raised any of the below as bugs yet. I don't know how defensive we want to be.

If you pass something that isn't a list to tunnelsArrayCompare (either directly to the function or through checkIfChanged):

TypeError: arr1.sort is not a function

If the list of tunnels contains something that isn't a dictionary in checkIfChanged:

TypeError: Cannot create property 'type' on string 'i%~pKE]`'
    at /srv/service/import-data-utils.js:101:16
    at Array.forEach (<anonymous>)
    at getTunnels (/srv/service/import-data-utils.js:99:10)

If you pass undefined or null to tunnelsCompare or tunnelsSort:

TypeError: Cannot read properties of undefined (reading 'operator')

(Obviously!)

Code

• Code for generating test input data: https://gitlab.wikimedia.org/dwalden/ipmasking-testing/-/blob/main/spur_random_data_valid_1.py
• Code for updating one field at a time: https://gitlab.wikimedia.org/dwalden/ipmasking-testing/-/blob/main/random_spur_data_updater_1.py
• Example of unit testing the individual functions (I had to copy them from output-sql.js): https://gitlab.wikimedia.org/dwalden/ipmasking-testing/-/blob/main/output-sql-fast-check.test.js
• Example of randomly generating the file that is input to output-sql.js: https://gitlab.wikimedia.org/dwalden/ipmasking-testing/-/blob/main/output-sql-fast-check-file.test.js
• Example of unit testing the individual functions, this time allowing the random data generator to generate any kind of data it knows how: https://gitlab.wikimedia.org/dwalden/ipmasking-testing/-/blob/main/output-sql-fast-check-anything.test.js

@Tchanders does this task need moving to the next sprint or can this be marked as resolved?

@Dreamy_Jazz I've moved it to next sprint - thanks

tchanders opened https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/195

Improve tunnel validation to avoid errors when diffing

kharlan merged https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/195

Improve tunnel validation to avoid errors when diffing

@Tchanders I received a type error: : Cannot read properties of undefined (reading 'count') after running diff.sh using the test data that was used previously.

Status: ❌Fail
Environment: VSC
OS: macOS Sonoma 14.2
Browser: N/A
Skins. N/A
Device: MBA M2
Emulated Device:: n/a
Test Links:
N/A

❌AC1: https://phabricator.wikimedia.org/T351197

Code for generating test input data: https://gitlab.wikimedia.org/dwalden/ipmasking-testing/-/blob/main/spur_random_data_valid_1.py

Steps

• python3 tmp/spur_random_data_valid_1.py > randomtest.json
• gzip -c randomtest.json > randomtest2.json.g
• docker-compose exec web ./diff.sh --today tmp/randomtest2.json.gz

TypeError: Cannot read properties of undefined (reading 'count')
    at generateInsertActorQueries (/srv/service/output-sql.js:174:33)
    at Interface.import (/srv/service/output-sql.js:363:6)
    at Interface.emit (node:events:513:28)
    at Interface._onLine (node:readline:491:10)
    at Interface._normalWrite (node:readline:665:12)
    at ReadStream.ondata (node:readline:272:10)
    at ReadStream.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at ReadStream.Readable.push (node:internal/streams/readable:228:10)

Change 989503 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[operations/deployment-charts@master] ipoid: Bump version

https://gerrit.wikimedia.org/r/989503

Change 989503 merged by jenkins-bot:

[operations/deployment-charts@master] ipoid: Bump version

https://gerrit.wikimedia.org/r/989503

@Tchanders I'm not sure if the reloaded patch was supposed to resolve but I'm still getting the same issue as previously when running those steps.

commit 47078469e00116422a82cca8c0b4770e4a8a12d3
Thu Jan 18 11:37:45 2024

Thanks @GMikesell-WMF - filed as T355896: output-sql.js throws an error if actor.client does not exist in the dataset.

Note that while testing this on the latest version of ipoid, I noticed that spur_random_data_valid_1.py creates data that does not have valid IPv4/6 addresses, meaning that it fails sooner.

Just mentioning here in case it wasn't already picked up.

Edit: it's not the latest version at the time of writing, but a branch that will solve this task: T354758: Data not retrieved by IPInfo from IPoid for IPv6 addresses - so it's not a problem yet at the time of writing, but it will become an issue soon.