Use lang tester's ignore-if to avoid encoding configuration in test names
#953
Conversation
We can now use -- with a suitably set environment variable -- lang_tester's `ignore_if` to avoid having to encode which tests we want to run in filenames.
| @@ -1,3 +1,4 @@ | |||
| // ignore-if: test YK_JIT_COMPILER != "yk" | |||
There was a problem hiding this comment.
Can we just pick up YKD_NEW_CODEGEN here?
https://ykjit.github.io/yk/dev/runtime_config.html#ykd_new_codegen
There was a problem hiding this comment.
YKD_NEW_CODEGEN is like calling a road "new road" -- when you build another road should it be called "newer road"? The directory structure in ykrt/compiler gives these things names, so we should IMHO use those names and not a boolean NEW_CODEGEN env var.
There was a problem hiding this comment.
Why don't we push a commit to rename the existing YKD_NEW_CODEGEN env var and use that instead? I don't see any benefit to having two environment variables for the same purpose.
There was a problem hiding this comment.
That's probably a good idea, but the YK_ precedent is already present in the test infrastructure, so renaming YKD_NEW_CODEGEN feels like a separate PR to me (and best done by someone who knows exactly what YKD_NEW_CODEGEN does).
There was a problem hiding this comment.
I agree. So this PR should follow precedent, and a follow up PR can do the rename.
|
Great. Simplifies a lot of stuff! Just one small comment. |
|
@vext01 I think this is OK to merge now? |
|
I meant this PR should use the existing env var in the interim, but don't worry, I'll sort it in a follow up PR :P |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Is it possible that some bad logic is causing all tests to be run if |
|
Right, every non-new-codegen test needs to be ignored if |
Hmm, yeah. Of course I didn't shake that out in local testing because I didn't turn that feature flag on. Oops. Let me rethink. |
|
@vext01 Hopefully that new commit fixes things. |
|
I can live with it. Please squash. |
ltratt
force-pushed
the
use_lang_tester_ignore_if
branch
from
a82a5af to
7abb8a6
Compare
|
Squashed. |
This PR, in three stages, removes the need to encode "only run this C test if X" using lang_tester's new
ignore-if. Note that we have to useset_envso that the environment variables are available toignore-if. Maybe lang_tester should grow the ability to have an environment just to runignore-ifcommands in -- but that's a worry for another day.