-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation
…not actual deps :)
- fixing the CONFIG_FIXTURESDIR path - ensuring that required dirs are created before creating a file in them - adding a default_eunit.ini file - making sure some required files are there before starting the tests
@@ -0,0 +1,27 @@ | |||
export BUILDDIR ?= /Users/elbrujohalcon/Projects/inaka/couchdb-config |
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.
License header should be applied for Makefiles as well.
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.
oops
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.
Default path also should be changed for something that will works for everyone.
The problem of this PR that it cannot solve dependency issues. If your main goal of these changes was to be able to run the test suite without external dependencies, then the test suite should be changed (actually, improved) in order to run unit tests, not integrational as it does now. In this case there would be no need in couch_eunit copy-paste, all external deps will be mocked and couchdb-config would be tested isolated from other CouchDB parts. |
@kxepal my goal was to be able to run the app without CouchDB. |
@elbrujohalcon I see. This is a good goal and I also want that (: In fact, for some apps that could not be so easy. |
@elbrujohalcon Thanks! But something went wrong:
Any ideas? |
It's basically saying cp: warning: source file â��test/config_tests.erlâ�� specified more than once I'm not sure why, but I pushed one commit more with a change that may or may not help. It's a very long shot, but just in case… Can you try again? |
clean: app | ||
${REBAR} clean | ||
|
||
test: app |
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.
We use make check
to run the tests for other subprojects. Could you add it here? Alias is fine.
@elbrujohalcon Last commit fixed that problem, thanks! |
|
{noreply, State}; | ||
handle_info(Info, State) -> | ||
couch_log:error("config:handle_info Info: ~p~n", [Info]), | ||
couch_log:notice("config:handle_info Info: ~p~n", [Info]), |
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.
These should be switched back to the original levels.
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.
oops, fixing that right away
I like where you're going with this @elbrujohalcon! One thing to be aware of, is that we run the test suite for this application as part of a parent application in http://github.com/apache/couchdb, so the duplication of |
Let's try that out, @chewbranca … wait for my next commit |
This whole thing lead me to detect a missing dep in couch-lager as well. Check PR apache/couchdb-lager#1, guys |
… commit to work, apache/couchdb-lager#1 must be merged first
|
Thanks @elbrujohalcon! If I use apache/couchdb-lager#1 all the tests pass. I'm +1 although I wonder if there's a way we can get rid of Also, given this is one of the first applications that's being modified to run outside of CouchDB itself, it would be good to get some feedback from @rnewson and @janl. |
\o/ Thanks @chewbranca for review! |
@@ -0,0 +1,18 @@ | |||
relx |
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.
we don't use relx or erlang.mk or Mnesia, where is this list coming from?
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's the generic .gitignore we use at @inaka for all erlang projects, I can trim it down.
The whole point of having separate repos was to ensure they'd be able to evolve independently, so I'm definitely for the idea. I think the addition of a Makefile and changes to rebar.config are a problem, though, for the two different situations where we build this. I'd also like to discover why we used error_logger here rather than couch_log. It doesn't appear to be to break a cyclical dependency, so I'm inferring it was done that way so that couchdb-config does not have a dependency on couch_log at all. We shouldn't revert that choice lightly. I hate to say it, but I'd like to hold off merging this until the 2.0 release. We can then think about how the build and test system can work in a more modular fashion. Unless someone here is going to verify that this change works both ways satisfactorily? |
@rnewson you're right, there are problems with running test suite from couchdb repo with this (and lager) PR applied:
While tests are passed when runs only from couchdb-config repository. |
I do like @rnewson’s take. 2.0 is a bit of a moving target when it comes to building and testing for a little while longer. I love where this work is going and it is going to make CouchDB a lot better. Are we cool leaving this to merge just after 2.x.x was branched? |
I have no objections to that, @janl :) |
Neat, just saw this and agree it would be nice. Definitely agree that we'll want to hold off until after 2.0. As for the logging things, I didn't see the error_logger version but that's fine for us in CouchDB land as couch_log installs an error_logger event handler and forwards to the same place. We just need to make sure that whatever error report we use can be formatted by couch_log which is more than fine. I'm pretty sure if its got a rebar.config and looks like an erlang app that make won't be run when we build downstream so that seems fine as well. |
This PR is a bit old, I think I'll just close it |
This PR includes a couple of tiny fixes that basically ensure that you can run and test the app by just cloning the repo and executing things like
make app
,make shell
and/ormake test