-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve test suite for bibtex #46
Conversation
As defined in BibTEX version 0.99b https://ctan.javinator9889.com/biblio/bibtex/base/btxdoc.pdf
Some changes in default parsing (snapshot), but results are still ok
Ping @matthewwiese |
If you're looking for additional testing material, try the Collection of Computer Science Bibliographies whose entries contain their BibTeX source. I discovered it thanks to this Academia StackExchange comment some time back. Also, I think increasing coverage of malformed input is important. Currently there appears to be only two cases (1, 2) of error checking; if the effort will be made to rewrite the package then making sure it fails gracefully is crucial. Otherwise, glad to see your enthusiasm on this! |
HI @matthewwiese : I tried to add more test on errors due to unbalanced braces, etc (see also #42 and UPDATE: Now not skipped in #47 |
Hi @coatless , did you have the chance of having a look on this? Regards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dieghernan I did! The upgrade to testthat 3e looks great! Only had a few minor notes. Happy to merge the test in as-is.
|
||
test_that("arrange.authors when family names have white space #6", { | ||
tmp <- tempfile(fileext = ".bib") | ||
entry <- "@Article{newspaper, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be written in a stand alone file within inst/bib/*
instead of directly inline with the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, already done except for issue #35 , that specifically ask for reading from a string, not a file
@@ -47,7 +47,8 @@ Depends: | |||
Imports: | |||
utils | |||
Suggests: | |||
testthat | |||
testthat (>= 3.0.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add yourself as an author in the DESCRIPTION
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you
test_that("Test book-full", { | ||
bib <- system.file("bib/xampl_standard.bib", package = "bibtex") | ||
|
||
out <- suppressMessages(read.bib(bib)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are suppressing diagnostic messages here? Should we just set verbose = FALSE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was for not cluttering the test results with messages. I have divided the xampl in two, one with messages (tested on the first tests) and another without them.
@@ -23,6 +23,15 @@ jobs: | |||
- {os: ubuntu-latest, r: 'devel', http-user-agent: 'release'} | |||
- {os: ubuntu-latest, r: 'release'} | |||
- {os: ubuntu-latest, r: 'oldrel/1'} | |||
- {os: ubuntu-latest, r: 'oldrel/2'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self, we should upgrade to the latest action calls.
Thanks @coatless, I made some minor amendments following your comments. Ready for merge |
This PR increases the test suite for
bibtex
package (Phase 1: Current state / Testing on #45 ). I tried to cover as many cases as I can, including also some additional .bib files for testing purposes:do_read_bib()
(tests/testthat/test-do_read_bib.R) reading non-standard BiBTeX entries that is used on https://github.com/ropensci/RefManageR (@mwmclean).The tests mainly covers the R source code. It seems to be that a lot of the C code is not currently used (¿?).
Additionally, I upgraded testthat version to 3 (this allows the use of snapshots, that would be useful for testing Phase 2) and improved the R-CMD-Check action to increase the number of platforms on which the package is tested.
Any further improvements on the testing welcomed.
PS: Note that neither R not C code has been modified so far.