Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@vitalybuka Thanks for tackling this. I've always wanted to have our unit tests be runnable on targets that aren't the host. This looks like the first step towards that.
The approach looks good but I'd like to see us type check the new run_under parameter.
llvm/utils/lit/lit/formats/googletest.py | ||
---|---|---|
24 | It looks like we require self.run_under to be a list but we never check that it is one. We should check the type here and fail if it's wrong with litConfig.fatal_error(). It looks like if emulator: emulator = shlex.split(emulator) currently ensures we pass in a list but this is not very obvious because at first I thought we were passing in a string. | |
157 | If self.run_under isn't a list then it'll cause problems here. |
@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?
sure, sorry, I didn't noticed comments.
I even can revert if there is serious issues.
@vitalybuka No problem. No need to revert. I don't think the patch will break anything that exists today but I would like to make sure the new run_under parameter gets typed checked so that it's easier for people to adopt it. Please create a follow up patch and add myself and @yln as reviewers.
I reverted as I'd like also to see which bots it brakes, but it landed with another commit which seriously broke clang compilation, so I may miss something in the noise.
llvm/utils/lit/lit/formats/googletest.py | ||
---|---|---|
157 | Having that it's trivial to fix the argument we can just do that instead of raising fatal error. |
It looks like we require self.run_under to be a list but we never check that it is one. We should check the type here and fail if it's wrong with litConfig.fatal_error().
It looks like
currently ensures we pass in a list but this is not very obvious because at first I thought we were passing in a string.