-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
Ignore --test
flag when it is an user-provided argument
#96926
Ignore --test
flag when it is an user-provided argument
#96926
Conversation
d202dcc
to
f03a54b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
77d6283), it works as expected.
Should be good to merge after applying suggestions.
Looks good to me, please squash the commits together so we can merge this. See PR workflow for instructions 🙂 |
d1254e1
to
d481669
Compare
Since the change was so small I made it again but on top of current master. Then force pushed it here. Is that enough? |
--test
flag when it is an user-provided argument
// Early return to ignore a possible user-provided "--test" argument. | ||
if ((strlen(argv[x]) == 2) && ((strncmp(argv[x], "--", 2) == 0) || (strncmp(argv[x], "++", 2) == 0))) { | ||
tests_need_run = false; | ||
return EXIT_SUCCESS; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this make godot --test -- --yolo
fail to run tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not, this will process in order so --test
would be seen and handled first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. But I agree that this is hacky and we should look for an alternative way to handle this.
Won't #90507 be missed though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to make it better, in the end I concluded that it's probably good enough for now.
This is getting somewhat hacky for a niche use case, I might be more in favor of reverting #90507 if this has such implications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick look at improving this, but indeed --test
gets processed before the regular argument parsing so it's naturally a bit hardcoded.
Should be fine for now, I might poke at it later to make this clear as I'm not fond of this TEST_MAIN_OVERRIDE
.
Thanks! |
Simple fix for edge-case #95922