Skip to content

Set query server env variable in Makefile for isolated eunit tests#1747

Merged
eiri merged 1 commit intomasterfrom
proper-env-in-isolated-eunit-runner
Nov 19, 2018
Merged

Set query server env variable in Makefile for isolated eunit tests#1747
eiri merged 1 commit intomasterfrom
proper-env-in-isolated-eunit-runner

Conversation

@eiri
Copy link
Copy Markdown
Member

@eiri eiri commented Nov 19, 2018

Overview

After #1602 we requite env variable COUCHDB_QUERY_SERVER_JAVASCRIPT to point to js query server. This patch adds the env var on Makefile's eunit target returning ability to run the isolated eunit tests without setting that var in each tests' setup.

Testing recommendations

Isolated tests with query server, e.g. make eunit apps=couch_mrview suites=couch_mrview_all_docs_tests should run locally without throwing "unknown query server" exceptions.

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@eiri eiri requested a review from wohali November 19, 2018 18:01
Comment thread Makefile
# target: eunit - Run EUnit tests, use EUNIT_OPTS to provide custom options
eunit: export BUILDDIR = $(shell pwd)
eunit: export ERL_AFLAGS = -config $(shell pwd)/rel/files/eunit.config
eunit: export COUCHDB_QUERY_SERVER_JAVASCRIPT = $(shell pwd)/bin/couchjs $(shell pwd)/share/server/main.js
Copy link
Copy Markdown
Contributor

@iilyak iilyak Nov 19, 2018

Choose a reason for hiding this comment

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

I think we can use ${BUILDDIR} instead of calling $(shell pwd) several times.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@iilyak I guess we can, but it's inconsistent with usage pwd everywhere else and feels kind of flimsy, setting env var in one step then depending on it in another.

Why do you want this change? It's not like here is a performance penalty we care about.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

but it's inconsistent with usage pwd everywhere else and feels kind of flimsy, setting env var in one step then depending on it in another.

I looked only at a diff and missed the fact that is used everywhere. It looked strange to see export BUILDIR and $(shell pwd) two lines bellow.

Why do you want this change?

It is just a suggestion I am fine with the way you wrote it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It looked strange to see export BUILDIR and $(shell pwd) two lines bellow.

and one line above too 😄

Well, I agree it's not the best way to write Makefiles and better way to go about it would be something like ROOTDIR := $(shell pwd) to expand current dir at top of Makefile and then just use make's var ${ROOTDIR} everywhere else. But this is a bit bigger change for a quick fix I intended to do, so since you don't have strong feelings about it I'll go ahead with how it is.

Copy link
Copy Markdown
Contributor

@iilyak iilyak left a comment

Choose a reason for hiding this comment

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

+1

@eiri eiri merged commit 090d17e into master Nov 19, 2018
@eiri eiri deleted the proper-env-in-isolated-eunit-runner branch November 19, 2018 20:10
@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 19, 2018

@eiri see #1748, you should have done Makefile.win at the same time.

@eiri
Copy link
Copy Markdown
Member Author

eiri commented Nov 19, 2018

@wohali yes, my fault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants