Closed skearnes closed 4 years ago
Merging #350 into main will increase coverage by
0.02%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #350 +/- ##
==========================================
+ Coverage 78.57% 78.60% +0.02%
==========================================
Files 21 21
Lines 2226 2229 +3
Branches 467 467
==========================================
+ Hits 1749 1752 +3
Misses 340 340
Partials 137 137
Impacted Files | Coverage Δ | |
---|---|---|
editor/py/serve.py | 87.88% <100.00%> (+0.11%) |
:arrow_up: |
@antonkast-google I seem to have broken the editor test, but the error log is very unhelpful: https://github.com/Open-Reaction-Database/ord-schema/runs/1098386017
Any ideas?
skearnes and I chatted offline. The timeout is probably an exception preventing the "ready()" signal from executing.
skearnes reports a 409 response rather than a timeout when running locally, suggesting some difference compared to the GitHub CI environment. We should probably correct that first.
@antonkast-google I'm pretty stuck now; after updating to use the docker file I'm getting the timeout error locally and on GitHub with no additional information.
Good progress. Can I repro on this branch?
If so, I'll go figure out how to see what's going on and report back.
When I run the editor test on this branch (skearnes:349), I see two passes and two error messages in test output.
$ git clone git@github.com:skearnes/ord-schema.git ord-schema-skearnes
$ cd ord-schema-skearnes/editor
$ docker build .
$ docker run -it -p 5000:5000 <hash_from_docker_build>
$ # In another shell:
$ node js/test.js
console> JSHandle@error
console> PASS http://localhost:5000/dataset/empty/reaction/0?user=test
console> JSHandle@error
console> PASS http://localhost:5000/dataset/full/reaction/0?user=test
In the output from docker run, I see various warnings from our validation logic and then eight errors of the form,
[05:40:54] SMILES Parse Error: syntax error while parsing: 5
[05:40:54] SMILES Parse Error: Failed parsing SMILES '5' for input: '5'
I do not see the hang.
Anton suggested using {headless: false}
in the args to puppeteer.launch()
, which allowed me to fix some missed variable renames. Still working on fixing the 409 FAIL result now, but the hang is gone.
Finally found the issue; there seem to be ghosts on my mac that disappear when I use linux. Once I started debugging on linux things moved quickly. @antonkast-google @connorcoley
Finally found the issue; there seem to be ghosts on my mac that disappear when I use linux. Once I started debugging on linux things moved quickly. @antonkast-google @connorcoley
FYI I blew away and re-cloned my local repo and now it works.
I saw evidence that the problem was a browser cache in puppeteer, the node package that runs Chrome for our tests.
We should remain on the lookout for false passes or false failures in the editor test. Also skearnes found a puppeteer option to disable caching that we should add in the editor test.js.
Fixes #349 Fixes #353
It's kind of horrible that the code to load/unload/add
Data
messages is repeated three times with slightly different class names, but that feels like part of a larger de-duplication refactor that isn't very high priority.