Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upRe-introduce changes from #1814 #1818
Conversation
adam3smith
added some commits
Feb 1, 2019
This comment has been minimized.
This comment has been minimized.
@retorquere could you give this a quick look to make sure I have this right -- I'm just taking your old change & testing for |
This comment has been minimized.
This comment has been minimized.
Yeah sorry about that! I've just woken up here, I'll look at those and run some tests of my own, and report back in a bit. |
This comment has been minimized.
This comment has been minimized.
(not that it's any kind of excuse for my error, but why didn't the test suite pick this up?) |
This comment has been minimized.
This comment has been minimized.
I see now, travis only does a lint check. This looks good to me. |
This comment has been minimized.
This comment has been minimized.
Yeah, more advanced CI for translators would be nice, especially since most translators already have built in unit tests (wouldn't have helped here because export translators don't). @zuphilip and @dstillman talked about this briefly when they implemented the current CI but I forgot where that was left. Obviously the overhead to run the tests is pretty significant, so this isn't a trivial problem. |
adam3smith
merged commit 4a58261
into
zotero:master
Feb 1, 2019
1 check passed
adam3smith
deleted the
adam3smith:bibtex-extra
branch
Feb 1, 2019
This comment has been minimized.
This comment has been minimized.
Already working on both. Current tests run in a second or two. |
This comment has been minimized.
This comment has been minimized.
(and I've already found a few errors in existing translators) |
This comment has been minimized.
This comment has been minimized.
There is a ticket for supporting export translators in Scaffold zotero/scaffold#103 and I would appreciate also automatic tests for export translators. I guess that now with the new translation-server it is more easily possible to include to run also automatically the test cases for each pull request or commit. As most PR are changing only a single translator, it is enough to test these test cases and that shouldn't take that long. |
This comment has been minimized.
This comment has been minimized.
Not sure what you're referring to here, but we have a ticket for CI-based testing of translator commits. This would be separate from the existing daily translator tests, though it would use the same Node-based testing functionality. (Web translators can easily fail when run that way because JS isn't run, but import/export tests should never fail.) If you're working on this, it might be worth posting to that ticket to make sure you're not doing unnecessary work. |
This comment has been minimized.
This comment has been minimized.
Done. I started working on it because I saw an easy path to do it, I had some time, and I'm really, really bummed (and sorry) that I submitted a PR to the translators that broke things. |
adam3smith commentedFeb 1, 2019
But this time without breaking stuff...