Include break time in timings

[bencomp]

If my naive change works, this fixes #437. However, I did not test this!

[zkamvar]

Thank you for this, @bencomp!

This is failing in part due to #457, but will be fixed once we merge in #458. The other reason why it would fail is because coerce_integer() sets a default of -5 (in R, bare numbers are usually floats and integers are written with an "L" following them to indicate a "long" integer, though that's not necessarily true anymore.... it's weird, I know) so that we can find the negative numbers and report missing items.

Once the suggestion is merged, it will pass checks and I will add a new test here to check that adding a break item will work. Once that happens, I will release it.

sandpaper/tests/testthat/test-get_syllabus.R

Lines 35 to 54 in 6989c39

	test_that("episodes missing question blocks and timings do not throw error", {
	writeLines(
	"---\ntitle: Break\nteaching: XX\n---\n\nThis should not error.",
	fs::path(tmp, "episodes", "break.md")
	)
	set_episodes(tmp, c(get_episodes(tmp), "break.md"), write = TRUE)
	(res <- get_syllabus(tmp, questions = TRUE)) %>%
	expect_warning("No section called 'questions'") %>%
	expect_warning("missing timings from 1 episode")
	expect_equal(nrow(res), 4)
	expect_equal(res$timings, c("00h 00m", "00h 12m", "00h 24m", "00h 34m"))
	expect_equal(res$percents, c("0", "35", "71", "100"))
	expect_equal(res$episode, c("introduction", "postroduction", "Break", "Finish"))
	expect_equal(fs::path_file(res$path), c("introduction.html", "postroduction.html", "break.html", ""))
	expect_equal(res$questions, c(rep(q, 2), "", ""))
	})

PS, I promise I will get to your other PRs 😬

[bencomp]

Thanks for your comments and help, @zkamvar! I had looked at the tests, but don't fully understand them yet – do they run in order and not reset to a default state between tests, or is there more magic going on with these unit tests?

[bencomp]

PS, I promise I will get to your other PRs 😬

No worries :)

[bencomp]

As the code highlighting suggested, break is a keyword in R. That means I cannot reference my_yaml$break in the way I do now.

[zkamvar]

Thank you for this! I'm getting a green light on https://github.com/carpentries/sandpaper/actions/runs/4995718012/jobs/8948100405?pr=455, so that means that all the tests pass with the state of the world as it is today.

I had looked at the tests, but don't fully understand them yet – do they run in order and not reset to a default state between tests, or is there more magic going on with these unit tests?

Good question! The tests under test/testthat/ are run in alphabetical order using the {testthat} package (see https://r-pkgs.org/testing-basics.html) via devtools::test() or devtools::check().

For this particular file, the tests are dependent on one another (e.g. another test below this one needs to acknowledge that there is a break episode and adjust the expected timings accordingly).

Because all of the tests need to work with a lesson, the first script to run is tests/testthat/setup.R, where a test lesson is created and stored in a temporary location for the duration of the test suite and a reset function is exposed for the tests.

Each test file will reset the lesson and run the tests from top to bottom. In this way, the tests within a file are somewhat dependent on one another because they explicitly work on the lesson files. I have attempted to minimize this, but there are some times when the side-effects were necessary.

[zkamvar]

The checks are not passing, but that's because of #457, which has been fixed in main, so I'm going to merge this and then add the test and your contribution recognition, @bencomp

Thank you!