Dynamic endpoints handlers #10
Conversation
chttpd hardcoded handlers are replaced with with dynamic url handlers, which are functions in a dynamically created module, assembled from priv/chttpd_handler.cfg files from all interested applications. For couched these are currently: chttpd, mem3 and global_changes. This is a special branch that pull the prepared branches (of the same name) of chttpd, mem3 and global_changes, which have the config files. Run 'make check' to test the handlers. They are implicitly tested by other tests and there is a whitebox callback test chttpd_handler_callback_test, which calls the dynamic functions directly and mocks the funs that they return. This tests precisely the relationship between the *_handler function clauses and the returned function. There are more tests to come, both testing the endpoints via http, and the configuration assembly. BugzID: 27037
BugzID: 27037
|
Note, I am adding less essential stuff, if unplanned for, in separate commits, as requested. |
There was a problem hiding this comment.
Please don't call ?assert from eunit tests generator: this makes debugging extremely hard:
chttpd_handler_callback_test: all_test_.................................................. *failed*
::error:function_clause
in function chttpd_db:handle_request/1
called as handle_request(x)
in call from chttpd_handler_callback_test:'-assertReturns/3-fun-1-'/2
in call from chttpd_handler_callback_test:assertReturns/3
chttpd_handler_callback_test: all_test_...favicon.ico.................................... [0.070 s] ok
chttpd_handler_callback_test: all_test_..._utils......................................... [0.071 s] ok
since ?assert* macros checks expression in runtime while ?_assert* doing this when eunit really starts to run the tests + it provides better traceback. See how couchdb-couch tests are done.
As requested. Added module comment justifying reload tests. BugzID: 27037
BugzID: 27037
|
All cool, but seems your logging is still there. Try this trick: And compare the output yours: And now: Looks better right? I'd forgot a bit how to make eunit produce the right output: it doesn't works right if setup receives list of functions, so I eventually have to rollback ecd4f77 - sorry for the false tip. Now you see which function got executed and you'll easily know which one fails. |
|
Thanks a lot for digging into it, I'll change accordingly, however, do we need to have "httpd_handler_callback_test:206:" before each output line? I don't know what apache/couchdb-chttpd does differently with the eunit setup but the same test source did not print these module:line hints before every line. Also, I am using io:format to prevent even more prefix in each line and I will try out your setup but all I ever found was that ?debugFmt inevitably adds to the line. I did not invent using io:format(user, ... that is from the Eunit docs, the way to produce screen output. I polished this output to be concise because I needed to find a race condition that only became apparent in the logs. From that I got to appreciate briefer, readable, cleaned up logs and modelled it after the etap output. I will try your proposal and go from there, then come back with what I found regarding ?debugFmt and leave it up to you. |
That will save a lot of time if something will go wrong since it directly points on the exact line in exact module where to start your search.
IIRC that's some eunit internal thing about using
Sorry if "reinvent" was sound a bit rude. The problem of io:format that it will always print things to stdout. As for ?debug, you can turn this output off after defining NODEBUG leaving whole report clean.
Sounds like you'll have to add another case on that (; Good luck with chasing RC - that's not an easy.
I see. Well, for eunit other tests are already uses the "default" output format, so if you think it could be improved, it need to be improved everywhere. |
|
Ok, thanks! I can set NODEBUG at the top of the test module? |
|
Yes, but better add |
|
I guess the NODEBUG proposal was a misunderstanding. I was looking for a way to create readable test output, as created during the test. Not sure what you actually meant with using NODEBUG, will leave that out for now. I'll otherwise rewrite as requested, to move forward. For my part I am unconvinced the test output needs line numbers added as it clutters the test source a bit, not making it more readable and adding a runtime indirection, if I understand that correctly, away from the intuitive assumption, for what it's worth. And adding the additional setup time using foreach reduces the test speed, for little gain, for all I can see. |
|
@hdiedrich if you're looking for fixing eunit output take a look on how formatters are done for it. For instance: https://github.com/seancribbs/eunit_formatters . Doing the same work with io:format isn't right in anyway.
Remove this info, break the tests, give someone to fix them and ask does this info could make his live simpler or not (: Chasing readability shouldn't hurt maintenance support. |
|
It's peripheral, I believe, but the output of the test could be helpful to indicate, which endpoint breaks, rather than which test function breaks. That's why I think it may have been more helpful before. It's not too important maybe, I changed it according to your preference. I certainly see your point. Would it be welcome to propose eunit formatters? |
If function name describes what exactly it tests (which endpoint for what behavior in your case), you'll hit two birds with single stone (;
I'm +1 on this. Dream on something colorful with slow tests highlight and nicer function name transforming into more readable form - that would be awesome! |
|
I'm happy with merge this. @rnewson @davisp @chewbranca what do you think? |
There was a problem hiding this comment.
Seems I miss that moment. Why not to reuse config and have all the handlers defined there? We already keep httpd ones there and this gives us dynamic control over them with no server restart.
|
@hdiedrich: Any updates on this PR? |
|
Hi, I need to review what the over all situation for this is at this point. Status is that there was a wish from Paul on how to better kick the handlers in, which I liked, but got stuck implementing it and kept going backwards finding out why until it became clear that I need to port my system tests over from the original implementation in dbcore that had met little love there. I then waited for integration tests from Russel and Ilya to be merged, so I don't double their effort but those got stuck, too IIRC. The way forward should be simple and fast and I'll be happy to finally get it merged. The start up, as it is, has the weakness that there needs to be knowledge about the modules in play, which defeats the intended uncoupling. The solution would be to just re-compile every step of the way when a new app is added during start up. That makes it fool proof and self-sufficient. Config, as Alex suggests, should therefore not be needed. There are merge conflicts in this PR now for rot but the core functionality was mergeable rather cleanly. |
|
@hdiedrich Hey! Config support suddenly is needed by the following reasons:
|
chttpd hardcoded handlers are replaced with with dynamic url handlers. This is a special branch that pull the prepared branches (of the same name) of chttpd, mem3, global_changes, setup_httpd, mango_httpd, which have the relevant config files. See chttpd/27037-5-dynamic-endpoint-handlers. This is a re-do of PR 10 for bitrot: apache/couchdb-chttpd#10 BugzID: 27037
|
The plan that the couchdb 2.0 sprint team agreed on is basically;
As to how we can support 'overlay' of an application's endpoints, I'm less sure. I suggest we get the above steps done first and then reassess. |
|
Dynamic handlers were landed based on @iilyak work. Could you close this PR please? |
4 participants
chttpd hardcoded handlers are replaced with with dynamic url handlers,
which are functions in a dynamically created module, assembled from
priv/chttpd_handler.cfg files from all interested applications.
For couched these are currently: chttpd, mem3 and global_changes.
This is a special branch that pull the prepared branches (of the same name)
of chttpd, mem3 and global_changes, which have the config files.
Run 'make check' to test the handlers. They are implicitly tested by other
tests and there is a whitebox callback test chttpd_handler_callback_test,
which calls the dynamic functions directly and mocks the funs that they
return. This tests precisely the relationship between the *_handler function
clauses and the returned function.
There are more tests to come, both testing the endpoints via http, and the
configuration assembly.
BugzID: 27037