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

tests are broken #86

Closed
romand opened this issue Jul 9, 2012 · 28 comments
Closed

tests are broken #86

romand opened this issue Jul 9, 2012 · 28 comments
Assignees
Labels

Comments

@romand
Copy link
Contributor

romand commented Jul 9, 2012

again
@iced could you please check they are not broken before commit?

@ghost ghost assigned romand Jul 9, 2012
@iced
Copy link
Contributor

iced commented Jul 9, 2012

I don't understand code in tests. Either rewrite them so they are readable or I can't fix them.

@iced iced closed this as completed Jul 9, 2012
@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

what part don't you understand?
please don't close the issue until tests pass

@romand romand reopened this Jul 9, 2012
@iced
Copy link
Contributor

iced commented Jul 9, 2012

all test-related code is messy and hard to read. also, most of the test doesn't test lib but underlying services.

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

ok, then this one is messy and hard to read, too: https://github.com/iron-io/iron_worker_ruby_ng/blob/master/test/test_basic.rb
could you please say since what line code becomes messy?

@iced
Copy link
Contributor

iced commented Jul 9, 2012

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

ok, could you please point the line?
subroutine 'cli' runs cli with provided arguments, returning cli output as result
we use it in tests to ensure that output matches corresponding patterns

@iced
Copy link
Contributor

iced commented Jul 9, 2012

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

and what is wrong with that? run cli, ensure log matches /Queued up.*"id":"(.{24})"/ so it have queued task and output smth that looks like id, match that id and use in next call

@iced
Copy link
Contributor

iced commented Jul 9, 2012

I do not want to read regexps especially when they are not needed.

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

/Queued up.*"id":"(.{24})"/ is not readable? sure?
suggest better way to get id of task ran by cli, to make https://github.com/iron-io/iron_worker_ruby_ng/blob/master/test/test_cli.rb#L41 possible

@iced
Copy link
Contributor

iced commented Jul 9, 2012

Better way - remove this assert. It relies on specific msg format returned by server and has nothing to do with lib it supposed to test.

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

if I remove this assert, I'll need to remove all further asserts: they use task id I parse
I don't think /Queued up.*"id":"(.{24})"/ is cryptic enough to leave most of cli untested

@iced
Copy link
Contributor

iced commented Jul 9, 2012

you only need to test that cli passed correct options to Code / Client. no need to queue even in this case.

@treeder
Copy link
Contributor

treeder commented Jul 9, 2012

@iced can't hurt to go all the way through the process can it? Why wouldn't we do that?

@iced
Copy link
Contributor

iced commented Jul 9, 2012

just because tests takes 15 to 30 mins on my local box now, so they are completely useless as I can't run them.

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

we agreed (several times) that it is ok to involve service in ng test
main arguments were

  • great complexity (sometimes impossibility) to test lib itself, without service
  • it is good thing, when bugs in service are revealed, which happened several times with ng tests

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

@iced
use https://github.com/iron-io/iron_worker_ruby_ng/blob/master/remote_test.rb
run tests at least before pushing big series of huge changes
don't just ignore tests, please

@iced
Copy link
Contributor

iced commented Jul 9, 2012

I'd prefer suite which tests lib itself and does it without queuing 100 workers just to test some unknown to me thing.

@iced
Copy link
Contributor

iced commented Jul 9, 2012

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

@iced I'd prefer such suite, too
but main arguments were

  • great complexity (sometimes impossibility) to test lib itself, without service
  • it is good thing, when bugs in service are revealed, which happened several times with ng tests

have you read it?

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

personally I run

remote_test.rb EXCLP=batch

to run tests remotely, without test_batch
it takes ~3min

@iced
Copy link
Contributor

iced commented Jul 9, 2012

  • great complexity - I can leave if whole suite will queue like 20-30 workers. not ~1000 like it does now.
  • make separate suite for service testing, not related to lib.

@iced
Copy link
Contributor

iced commented Jul 9, 2012

  1. EXCLP isn't documented anywhere and i wasn't really aware about it. also, it got bad cryptic name :)
  2. 3 mins is way too much anyway.

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

do you mean you'll proceed to ignore tests?
just because of regular expressions and 3-min-too-much reason?

@iced
Copy link
Contributor

iced commented Jul 9, 2012

I tried to use them and still use them before release, but I only check if not MUCH of them fails. I never was able to make suite to run with 100% asserts passed. And don't make me start on test suite needing pry gem for yet another unknown for me reason.

I have my own set of workers (like 10 of them) which I queue to check if everything works right. It takes like 20-30 seconds and tests as much as test suite itself.

@romand
Copy link
Contributor Author

romand commented Jul 9, 2012

do I get it right?

  1. you refuse to mind the tests
  2. you have your own hidden non-automated test suite

@iced
Copy link
Contributor

iced commented Jul 9, 2012

  1. if they run more than min or two and some of them fail for missing gem (pry, while it's installed) - I don't care much about them.
  2. it's not automated, it's just set of workers and simple sh script which runs iron_worker upload / queue / log and cats all results to file which I diff with good one.

romand added a commit to romand/iron_worker_ruby_ng that referenced this issue Jul 10, 2012
romand added a commit to romand/iron_worker_ruby_ng that referenced this issue Jul 10, 2012
romand added a commit to romand/iron_worker_ruby_ng that referenced this issue Jul 10, 2012
romand added a commit to romand/iron_worker_ruby_ng that referenced this issue Jul 10, 2012
@iced
Copy link
Contributor

iced commented Sep 5, 2012

they pass now.

@iced iced closed this as completed Sep 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants