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

Show errors on console under plugin development #1302

Merged
merged 5 commits into from
Nov 2, 2016

Conversation

tagomoris
Copy link
Member

@tagomoris tagomoris commented Nov 1, 2016

This change fixes code to show error/fatal logs in testing console.

Raising errors doesn't help situation because:

  • errors raised in threads will be raised in main thread by abort_on_exception=true
  • main thread of test-unit will rescues and ignore errors

So we can't find any errors in unit tests without showing these on console.

I found that some #try_write method call #commit_write in it, and it misses to remove the chunk from @dequeued_chunks. It's a kind of bug.

Adding a chunk into @dequeued_chunks after calling #try_write has thread-unsafe bug. (because plugin may commit the chunk in another threads just after #try_write called, and plugin's main thread may have NOT added that chunk into @dequeued_chunks.)
So, I fixed it at the same time.

Raising errors doesn't help situation because:
* errors raised in threads will be raised in main thread by abort_on_exception=true
* main thread of test-unit will rescues and ignore errors

So we can't find any errors in unit tests without showing these on console.
@tagomoris tagomoris added the v0.14 label Nov 1, 2016
@tagomoris tagomoris force-pushed the show-errors-on-console-under-plugin-development branch from fd8d86f to 34dc637 Compare November 2, 2016 04:10
@tagomoris
Copy link
Member Author

@repeatedly could you review this change?

@@ -66,6 +66,12 @@ def thread_create(title)
yield
thread_exit = true
rescue Exception => e
if @under_plugin_development
STDERR.puts "\nError raised in thread from #thread_create, #{e.class}:#{e.message}"
Copy link
Member

Choose a reason for hiding this comment

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

Head \n is needed?
Previous log doesn't have \n?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this code expects that leading text is '.' (means progress of test cases) from test runner, without tailing newline.

if state.thread && state.thread.status # "run"/"sleep"/"aborting" or false(successfully stop) or nil(killed by exception)
state.thread.run
else
log.warn "thread is already dead"
Copy link
Member

Choose a reason for hiding this comment

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

If one thread is dead, we should replace dead thread with new active one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Dead flush thread means any kind of bug of output.rb.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

Off topic. Collecting live and dead threads information is good for debug.
If one thread is dead, it means other thread also becomes dead.
If this situation continues, all output threads are dead.
We should display the message when all output threads are dead.

@repeatedly
Copy link
Member

repeatedly commented Nov 2, 2016

Commented. Changes are LGTM.

BTW, I first knew out_stdout supported async write ;p

@tagomoris
Copy link
Member Author

OK, let me merge this later.

@tagomoris tagomoris merged commit 54d3509 into master Nov 2, 2016
@tagomoris tagomoris deleted the show-errors-on-console-under-plugin-development branch November 2, 2016 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants