-
Notifications
You must be signed in to change notification settings - Fork 170
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
[t] use one base class #458
Conversation
Generated by 🚫 Danger |
361a2b0
to
7e158ea
Compare
apicast/src/configuration.lua
Outdated
|
||
local empty_t = readonly_table() | ||
|
||
local function backend_endpoint(proxy) |
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.
The indentation here is not consistent with the rest of the codebase.
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.
Indeed. Haven't configured my IDE and used the default 4 spaces indentation.
Must admit that I quite like it. Somehow it makes it more readable. Also the OpenResty code is indented with 4 spaces.
I'd consider switching to 4 spaces. Wdyt?
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 don't mind as long as it's spaces :) and we are consistent across the codebase.
http://lua-users.org/wiki/LuaStyleGuide
|
||
my $pwd = cwd(); | ||
my $apicast = $ENV{TEST_NGINX_APICAST_PATH} || "$pwd/apicast"; | ||
use TestAPIcast 'no_plan'; |
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.
What is no_plan
?
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 don't really know. I guess it defines in what order to execute the tests. Search for plan
on http://search.cpan.org/~agent/Test-Nginx/lib/Test/Nginx/Socket.pm
https://openresty.gitbooks.io/programming-openresty/content/ also have few mentions of it.
env_to_nginx( | ||
'RESOLVER' | ||
); | ||
master_on(); | ||
log_level('debug'); | ||
repeat_each(1); |
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.
Should we define TEST_NGINX_REPEAT_EACH
instead?
From TestAPIcast: repeat_each($ENV{TEST_NGINX_REPEAT_EACH} || 2);
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.
Some tests force one iteration because 2nd one would hit cache and have different result.
Because the repeat_each is in the base this is an override. So all tests default to 2 (or the env var) except few ones that are forced to one.
@@ -0,0 +1,74 @@ | |||
package TestAPIcast; |
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.
Nice DRY 👍
@@ -45,6 +45,7 @@ jobs: | |||
working_directory: /opt/app-root/apicast | |||
steps: | |||
- checkout | |||
- run: rm -rf lua_modules |
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.
Why is this needed now, but not before? 🤔
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.
It is needed for local run. There is no way to ignore folders on local build.
unify all tests to use one base
so we can share setting between tests