Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
+19 −11
Closed

Debug mode tests #3293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a66b77c
test: pass --mode=$(BUILDTYPE) to test runner
bnoordhuis Oct 9, 2015
14e9d61
build: skip release build when BUILDTYPE=Debug
bnoordhuis Oct 9, 2015
890b79e
fixup! set NODE=node_g when BUILDTYPE=Debug
bnoordhuis Oct 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@ OSTYPE := $(shell uname -s | tr '[A-Z]' '[a-z]')
EXEEXT := $(shell $(PYTHON) -c \
"import sys; print('.exe' if sys.platform == 'win32' else '')")

NODE ?= ./node$(EXEEXT)
NODE_EXE = node$(EXEEXT)
NODE_G_EXE = node_g$(EXEEXT)

ifeq ($(BUILDTYPE),Release)
NODE ?= ./$(NODE_EXE)
else
NODE ?= ./$(NODE_G_EXE)
endif

# Flags for packaging.
BUILD_DOWNLOAD_FLAGS ?= --download=all
BUILD_INTL_FLAGS ?= --with-intl=small-icu
Expand All @@ -34,7 +39,7 @@ V ?= 1
ifeq ($(BUILDTYPE),Release)
all: out/Makefile $(NODE_EXE)
else
all: out/Makefile $(NODE_EXE) $(NODE_G_EXE)
all: out/Makefile $(NODE_G_EXE)
endif

# The .PHONY is needed to ensure that we recursively use the out/Makefile
Expand Down Expand Up @@ -88,15 +93,17 @@ cctest: all
@out/$(BUILDTYPE)/$@

test: | cctest # Depends on 'all'.
$(PYTHON) tools/test.py --mode=release message parallel sequential -J
$(PYTHON) tools/test.py --mode=$(BUILDTYPE) -J \
message parallel sequential
$(MAKE) jslint
$(MAKE) cpplint

test-parallel: all
$(PYTHON) tools/test.py --mode=release parallel -J
$(PYTHON) tools/test.py --mode=$(BUILDTYPE) -J parallel

test-valgrind: all
$(PYTHON) tools/test.py --mode=release --valgrind sequential parallel message
$(PYTHON) tools/test.py --mode=$(BUILDTYPE) --valgrind \
sequential parallel message

test/gc/node_modules/weak/build/Release/weakref.node: $(NODE_EXE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be just $(NODE) now?

$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
Expand Down Expand Up @@ -133,7 +140,7 @@ test/addons/.buildstamp: $(ADDONS_BINDING_GYPS) | test/addons/.docbuildstamp
build-addons: $(NODE_EXE) test/addons/.buildstamp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for this one, and possibly others

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't touch that yet because $(NODE) is either a release build or a debug build, whereas $(NODE_EXE) is always a release build.

The add-on tests all expect the shared object in out/Release but node-gyp puts the add-on in out/Debug when it detects a debug build. I decided to put off fixing the tests until it was clear if this is the direction we're going to take.


test-gc: all test/gc/node_modules/weak/build/Release/weakref.node
$(PYTHON) tools/test.py --mode=release gc
$(PYTHON) tools/test.py --mode=$(BUILDTYPE) gc

test-build: | all build-addons

Expand All @@ -144,8 +151,9 @@ test-all-valgrind: test-build
$(PYTHON) tools/test.py --mode=debug,release --valgrind

test-ci: | build-addons
$(PYTHON) tools/test.py -p tap --logfile test.tap --mode=release --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) addons message parallel sequential
$(PYTHON) tools/test.py -p tap --logfile test.tap --mode=$(BUILDTYPE) \
--flaky-tests=$(FLAKY_TESTS) $(TEST_CI_ARGS) \
addons message parallel sequential

test-release: test-build
$(PYTHON) tools/test.py --mode=release
Expand Down Expand Up @@ -175,11 +183,11 @@ test-npm-publish: $(NODE_EXE)
npm_package_config_publishtest=true $(NODE) deps/npm/test/run.js

test-addons: test-build
$(PYTHON) tools/test.py --mode=release addons
$(PYTHON) tools/test.py --mode=$(BUILDTYPE) addons

test-timers:
$(MAKE) --directory=tools faketime
$(PYTHON) tools/test.py --mode=release timers
$(PYTHON) tools/test.py --mode=$(BUILDTYPE) timers

test-timers-clean:
$(MAKE) --directory=tools clean
Expand Down
2 changes: 1 addition & 1 deletion tools/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
Copy link
Member

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

Copy link
Member Author

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Release' => ['release']?

options.run = options.run.split(',')
if options.run == [""]:
options.run = None
Expand Down