Skip to content
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

Debug symbols tests #6

Merged
merged 23 commits into from
Jul 1, 2022
Merged

Debug symbols tests #6

merged 23 commits into from
Jul 1, 2022

Conversation

FabrizioSandri
Copy link
Owner

@FabrizioSandri FabrizioSandri commented Jun 18, 2022

This pull request is meant to implement CI test for the pull request #3.

Description

We want to see if debug symbols are included by default in the generated library when invoking the command R CMD INSTALL on specific platforms; in this example, Arch Linux-based systems. To test this, I'll remove the -g option, which was added in pull request #3 and run CI.

@FabrizioSandri
Copy link
Owner Author

As we can see CI failed with the message ERROR: The binaries are missing debug symbols. In fact, the compiled binaries for the testSAN package are missing them. The presence of debug symbols is checked by the GitHub Action using the file command:

file ./inst/testpkgs/testSAN/src/read_out_of_bound.o 

which returns the following result when the system is not including debug symbols

src/read_out_of_bound.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped

When debug symbols are available, however, the string with debug_info is added, as seen in the following result.

src/read_out_of_bound.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), with debug_info, not stripped

Let's restore the changes made in the pull request #3 and check if CI works.

@FabrizioSandri
Copy link
Owner Author

CI is no longer failing, and the issue has been resolved.

@tdhock
Copy link
Collaborator

tdhock commented Jun 19, 2022

looks like a good start @FabrizioSandri
I really appreciate your diligent PR comments, and the fact you remove/restore the relevant code so we can see the red x change to green check.
A few comments

  1. usually in an R package we would use a testing framework/package like tinytest or testthat, with the tests cases written in R code files (here your test case is in a shell script). Do you think it would be possible to make that change?
  2. Also I see your test case uses grep debug_info to determine pass/fail status based on what was printed to standard output. That is ok but it seems like it would be even better if an R object could be examined to determine pass/fail status, is that possible?
  3. can you use R CMD check to determine pass/fail status of CI? I think that is what people usually do via the r-lib/actions/check-r-package, see https://github.com/tdhock/binsegRcpp/blob/master/.github/workflows/R-CMD-check.yaml for an example.

@FabrizioSandri
Copy link
Owner Author

I'm seeing new issues while developing the new GitHub action.

The first is concerning Roxygen code examples that don't work for the deepstate_compile_fun function since it requires the harness files.I fixed it by calling the deepstate_pkg_create function before running deepstate_compile_fun.

The second relates to testthat. I discovered two more issues while running the tests for the library in the RcppDeepState/tests/testthat/test-new-test.R file:

  • The first is that the deepstate_create_makefile function, which creates the makefiles, calls the install.packages function without providing the repos at line 61. If the user does not provide a CRAN mirror, the following error will occur:
Error in `contrib.url(repos, type)`: trying to use CRAN without setting a mirror
  • In addition, I added the fixed=TRUE argument to an expect_error invocation. This argument must be provided to prevent the text being processed as a regular expression makinf the test fail.

@FabrizioSandri
Copy link
Owner Author

In the last four commits I have done the following changes:

  • removed the old tests data, otherwise running the R CMD check twice will fail. The test named Missing testfiles will fail because the files already exists.
  • the line that tries to install the tested package has been commented out. In fact the installation step is not required; all tests will be run without using any of the library's R constructs, instead relying on the contents of the src folder.
  • the Makevars generating part has been updated. The Makevars file should be created only if the package uses Rcpp; in other word, if it contains an src folder. If this folder doesn't exists, creating the Makevars file will end up with an error.
  • setup a basic CI workflow using r-lib/actions to run R CMD check.

Next steps: implement the check for debug symbols using the testthat suite.

@FabrizioSandri
Copy link
Owner Author

While searching for a way to perform non deterministic fuzz testing with RcppDeepState by using some kind of seeds, I discovered that the seeds of DeepState would not work if the RInside declaration is made inside the TEST macro of the test Harness. I've written a detailed description of the problem, along with an example, which you can find here.

Finally in the last commit I've introduced a seed argument to the deepstate_harness_compile_run function. This way it is possible to provide a seed for the test. If the seed isn't provided, the default random seed will be used.

This was referenced Jun 21, 2022
@FabrizioSandri
Copy link
Owner Author

With the last few commits, I have been able to fix the issue that the inputs of some functions are not generated. The problem was due to a Segmentation fault that prevented DeepState to create the output files. A possible solution for this problem is to separate the test harness into two different tests:

  • generator: used to generate the new input data, without running the function being tested
  • runnner : used to actually run the function being tested, passing the inputs generated by the generator.

By doing this way we ensure that no error prevents the input of being generated.

This solves the Issue #7

@FabrizioSandri
Copy link
Owner Author

@tdhock I included in the last commit the check for debug symbols. Simply put, the test uses a predefined seed that has been shown locally to uncover some errors in the testSAN package. If the result table for the analysis is empty, the library wasn't built with the -g option.

@tdhock
Copy link
Collaborator

tdhock commented Jun 30, 2022

great thanks.

@FabrizioSandri FabrizioSandri merged commit 6fa4448 into master Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants