Allow tests to be disabled for specific gems; warn about disabled tests - #6012
Merged
matz merged 1 commit intoMay 16, 2023
Merged
Conversation
matz
added a commit
that referenced
this pull request
Jul 17, 2023
If `mrbgems` is included in the parent directory path (MRUBY_ROOT), it confuses gem path and build path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
While it is preferable to leave all tests enabled, I had a need to disable tests for specific mgems while leaving all others enabled. For example, some mgems which my project requires contain unit tests that depend on remote servers, and the tests will always fail if those servers are not accessible (e.g. due to firewall restrictions or temporary internet outage). Thus, this PR makes it possible to disable some tests while leaving the rest of the test suite enabled.
Syntax
I think the assignment-style syntax,
g.skip_test = true|false, might allow for more flexibility within a complex build script, but if a more declarative style (g.skip_test) is preferred, this is an easy change that I can make.Implementation Notes
If
mruby-test/mrbgem.rakeencounters a gem with disabled tests, it simply does not generate a call toGENERATED_TMP_mrb_#{g.funcname}_gem_test(mrb). Instead, it adds a warning to$assertsso that the builder won't forget that they have disabled these tests.I considered also suppressing the generated test code in
gem_test.c. If we did that, the generatedmrbtestbin should be smaller. However, I decided to leave it in place, in case the builder wants to call the test functions directly for some reason (perhaps with a custom test driver, for example). Therefore, in this PR the test logic is still generated and its symbols are still exported as before, but it just isn't executed by the defaultmrbtest.In order to add the warnings to
$asserts, I had to#include <mruby.h>(and a few others) in the generatedmrbtest.cand deletetypedef struct mrb_state mrb_state. I am not sure if there was a specific reason that the original version relied upon a typedef instead of includingmruby.h, so if a dependency onmruby.his not desirable, then we could create a function indriver.cand call that function instead.Testing
I did give some thought to writing a test case for the proposed behavior. I considered creating a dummy mgem, which contains a single assertion that always fails. Then, I could modify
build_config/ci/*.rbto disable the dummy mgem test withskip_test = true. When the test passes, the dummy test would be disabled and therefore not generate a failing assertion. However, I decided to wait and see what you think about this approach. It seems heavy handed and maybe you know a better way. Also, I'm not sure where a dummy gem should live (wouldtest/be a good location?). I am happy to add this kind of test case if desired.