test-nginx icon indicating copy to clipboard operation
test-nginx copied to clipboard

Put load_module directives before the stream block

Open choroba opened this issue 7 years ago • 8 comments

Fixes #69.

choroba avatar Nov 04 '18 22:11 choroba

This is just a wild guess. Feel free to correct me if it's wrong or too hacky.

choroba avatar Nov 04 '18 22:11 choroba

@thibaultcha Will you have a look at this. If it looks good to you, then feel free to merge this :) Thanks!

agentzh avatar Nov 05 '18 01:11 agentzh

One concern is the 1 offset in the line numbers for the subsequent lines. This may break existing tests.

agentzh avatar Nov 06 '18 00:11 agentzh

@agentzh Tests usually refer to the Lua snippet within the *_by_block directive though, don't they? Are there other tests that rely on the nginx.conf line numbers themselves? This might be unlikely, otherwise the insertion of the env directives would constantly offset the generated config from different systems running them, wouldn't they?

thibaultcha avatar Nov 06 '18 00:11 thibaultcha

@thibaultcha Some tests may reference nginx.conf line numbers printed in the error log messages, like this:

runtime error: access_by_lua(nginx.conf:37):2: bad bad bad

agentzh avatar Nov 06 '18 03:11 agentzh

@agentzh Indeed, I have found the following cases of tests such as you described:

./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:22/
./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]==]" in .*?\nginx\.conf:22/
./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:22/
./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]=]" in .*?\bnginx\.conf:22/
./lua-nginx-module/t/158-global-var.t:rewrite_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:access_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:content_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:log_by_lua\(nginx\.conf:50\):3: in main chunk\n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:content_by_lua\(nginx\.conf:56\):4: in\n\z/, "old foo: 1\n"]
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]==]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]=]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/142-ssl-session-store.t:qr/\[emerg\] .*? "ssl_session_store_by_lua_block" directive is not allowed here .*?\bnginx\.conf:28/

If you don't mind, I would like to update them to use nginx\.conf:\d+ instead, since imho, relying on the nginx.conf line number is unsafe and should be subject to changes (what if we'd like to introduce a new directive, such as a new env variable in the future?). Third-party test suites (external to the OpenResty organization) should probably not assume that the internal nginx.conf is not subject to changes either...

Once updated, we can safely apply my suggestion and merge this. What do you think?

thibaultcha avatar Nov 06 '18 08:11 thibaultcha

@thibaultcha Actually some tests really want to check the nginx.conf line numbers. That's part of the things we want to check accurately.

agentzh avatar Feb 21 '19 20:02 agentzh

I think it is fine since we don't use load_module in our tests.

spacewander avatar Mar 12 '21 02:03 spacewander