-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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 mode tests #3293
Debug mode tests #3293
Conversation
@@ -1304,7 +1304,7 @@ def ProcessOptions(options): | |||
global VERBOSE | |||
VERBOSE = options.verbose | |||
options.arch = options.arch.split(',') | |||
options.mode = options.mode.split(',') | |||
options.mode = options.mode.lower().split(',') # 'Release' => 'release'. |
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 this comment really adds value here
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 figured that it wasn't entirely obvious from context what options.mode is and why it needs to be lower-cased. I can remove it if you feel strongly about it.
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.
'Release' => ['release']
?
There's a few others in there that could have |
@bnoordhuis good stuff. Changes generally LGTM, though the add-ons tests still need to be fixed. Prototype job run here: https://ci.nodejs.org/job/orangemocha-test-commit-linux/2/ (still pending) |
@bnoordhuis ... ping |
7da4fd4
to
c7066fb
Compare
c133999
to
83c7a88
Compare
Refs #3280.
R=@orangemocha
I'll spruce up the commit logs but this is the basic idea of what I mentioned here.