Conversation
Updated remote repository reference for formatters. Signed-off-by: Joe Zhu <sha.joe.zhu@gmail.com>
Signed-off-by: Joe Zhu <sha.joe.zhu@gmail.com>
…j_templates_scripts
331456d to
8133017
Compare
Signed-off-by: David Muñoz Tord <david.munoztord@mailbox.org>
Unit Tests Summary 1 files 274 suites 14m 39s ⏱️ Results for commit 681ae5b. ♻️ This comment has been updated with latest results. |
Unit Test Performance DifferenceTest suite performance difference
Additional test case details
Results for commit 2e8a10e ♻️ This comment has been updated with latest results. |
|
hi @munoztd0 , thanks for submitting the PR, i would suggest to snapshot the ascii tables instead of rtf, the table is much more readable for comparing on github, rtf is less ideal in this case. i am guessing you would like to use rtf due to output formatting? |
@shajoezhu I 100% agree but you're also right on your guess. If that is a blocker I could put it up the food chain but I have no hope for that. |
|
New error on tsfae13 appeared but only on the CRAN pass, still need to investigate |
|
Hi @shajoezhu @Melkiades, The rtf files are snapshotted as text, rather than as binary files. Specifically with Note this prevents issues issues with OS-specific line endings (guess why this is how we test them...). This should prevent the most egregious case of Furthermore, these files are not generated by third party libraries with OS-specific behaviors, but rather by concatenation of strings in {tidytlg} (this has its own issues, and I have ... feelings about it, but exact text brittleness is not one of them). The issue with just testing the table is that the because J&J doesn't use ASCII for their submissions, the table is insufficient to prevent regressions from our submission-requirement specs. If I had my way this wouldn't be an issue but it's multiple orders of magnitude above my pay grade and simply put, 100% not going to change. I'd like to provisionally include the rtf files in the tests and then if we are getting (unexpected) changes we can take the full snapshots out and fall back to an imperfect backup plan. My issue with doing string checking for the (unfortunately for us) mandatory decorations/formatting is that that is likely to be substantially more brittle and prone to false negatives (ie tests passing when they shouldn't) than the snapshot tests. We can keep this in our pocket as a back up, as I alluded to above, but I'd like to try the better solution first, with the expectation that it won't have the Let me know what you think. I'll also discuss this with you, Joe, when we meet next if desired. |
|
hi @gmbecker and @munoztd0 , I would like to request the following test, and better understand the limitation (if there is any) using rtf and see how much impact that the future result I would like to propose several tests and branch off from this jnj_templates_scripts, and propose merge request, and see the differences of changes
or above should be in 4 seperate branches, so it would be eaiser for us to investigate |
Signed-off-by: David Muñoz Tord <david.munoztord@mailbox.org>
…templates_scripts
|
hi @munoztd0 , thanks for the PRs, I can see you have put in a lot of efforts into these test cases. i was wodnering if you can update with your rtf snapshot changes please? |
|
Hi @gmbecker and @shajoezhu, thanks for the detailed set of proposed tests. First off: sorry for the mess/noise and all the notifications. Managing multiple PRs plus the CI/CD runs ended up being a bit of a nightmare 😅, but I’ve now completed the requested investigations and kept each experiment isolated to make review easier. PRs created (one per test)1) Template-level rounding change decimals
2)
|
As discussed we want to integrate JnJ innovative medecine newly release open-source TLG catalog scripts.
Any feedback would be very welcomed.