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.

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

test: reformatted the test cases by latest tool reindex. #22

Closed
wants to merge 5 commits into from

Conversation

membphis
Copy link
Contributor

No description provided.

@membphis
Copy link
Contributor Author

membphis commented Jul 12, 2019

BTW, https://github.com/Kong/lua-resty-healthcheck/blob/master/t/14-tls_active_probes.t#L26

I can not find this path in my centos 7 OS. here is my file list:

$ ls /etc/ssl/certs/ca-*
/etc/ssl/certs/ca-bundle.crt  /etc/ssl/certs/ca-bundle.trust.crt

Copy link
Contributor

@hishamhm hishamhm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Please fix the typo in the commit message and remove the Perl reindex tool from this PR due to copyright reasons. You're welcome to include the numbering fixes from reindex.

Also, recently, this repository standardized in using Kong-style commit messages (type(scope) message) instead of OpenResty-style (type: message.). This is not a blocker for this PR, but adjusting this would be welcome (git rebase -i master is your friend for that!). These are suggested commit messages:

  • 1c4: chore(reindex) reformat the test suite
  • d72: tests(active_probes) use ca-bundle.trust.crt if fails to find ca-certificates.crt (and squash commit cfb in this one)
  • 70b: perf(healthchecks) cache local variables

@@ -1011,7 +1023,7 @@ end
-- Log a message specific to this checker
-- @param level standard ngx log level constant
function checker:log(level, ...)
ngx_log(level, worker_color(self.LOG_PREFIX), ...)
return ngx_log(level, worker_color(self.LOG_PREFIX), ...)
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not seem necessary, as we don't care about the return of ngx.log (is there any? the docs don't say so)

Copy link
Contributor Author

@membphis membphis Jul 13, 2019

Choose a reason for hiding this comment

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

We use lua's tail call feature here so that we can see the caller's line number in the error log.

please take a look at this example, pay more attention about the output of error log:

$ resty -e 'local function log1(...)
                ngx.log(ngx.ERR, ...)
            end

            local function log2(...)
                return ngx.log(ngx.ERR, ...)
            end

            log1("first")
            log2("second")'
2019/07/12 21:04:01 [error] 40647#40647: *2 [lua] (command line -e):2: log1(): first, context: ngx.timer
2019/07/12 21:04:01 [error] 40647#40647: *2 [lua] (command line -e):10: second, context: ngx.timer

I think we more love the implementation of log2, which can record the original source code line number.

Copy link
Member

Choose a reason for hiding this comment

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

@membphis that is a neat trick to get the proper line logged. It has been very annoying to only get the lines from the log function. Definitely one to remember and reuse elsewhere. Thx.

Copy link
Contributor

Choose a reason for hiding this comment

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

@membphis @Tieske while Lua does have a tail call feature, losing the line number info when performing a tail call is a LuaJIT-specific (mis)behavior. Here's an equivalent non-resty specific script:

local function x()
   return debug.getinfo(1, "l")
end

local function y()
   local x = debug.getinfo(1, "l")
   return x
end

print(x().currentline)
print(y().currentline)

With Lua 5.3 this prints "2 6", as it should; with resty this prints "10 6".

It may be a "neat trick" that you managed to get a useful behavior out of the fact that LuaJIT loses debugging information, but I would be wary of relying on such undefined behavior. A future version of the compiler may fix this bug and then line number info will change in surprising ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, I'm not saying this change should be removed from this PR — just that I'm not a fan of it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that we can keep this improvement. After all, the project works in the openresty environment, we can only use luajit.

t/util/reindex Outdated Show resolved Hide resolved
lib/resty/healthcheck.lua Show resolved Hide resolved
@membphis
Copy link
Contributor Author

@hishamhm I have rebased this PR right now, mainly modified the style of commit log.

@membphis membphis closed this Nov 7, 2019
@membphis membphis deleted the test-style branch November 7, 2019 07:42
AlinsRan added a commit to AlinsRan/lua-resty-healthcheck that referenced this pull request Jun 5, 2023
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