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

Isolate each test in a parentless module. #17435

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Conversation

yuyichao
Copy link
Contributor

Replaces #9798.
This uses a parentless module (similar to #9798) since I was interested in the memory saving (not much but I haven't measured too carefully).

There are two test breakage that I need to workaround (instead of fixing).

  1. Defining a new module on one process is not visible on another one, which breaks the DictChannel test in examples.
  2. The dictionary key completion doesn't work for qualified names.

There might be a way to fix the first one (or just disable isolation for it as what I'm doing now is probably good enough). The second one is likely a bug that should be fixed.

@yuyichao yuyichao added the test This change adds or pertains to unit tests label Jul 15, 2016
@yuyichao yuyichao changed the title Isolate each test in an parentless module. Isolate each test in a parentless module. Jul 15, 2016
@yuyichao yuyichao force-pushed the yyc/tests/isolate branch from 33e977f to 2d1f4fd Compare July 15, 2016 13:52
@yuyichao
Copy link
Contributor Author

I pushed a change to make the example test pass while being isolated by running the remote on the same worker. I'm not sure if it is still testing everything needed though. @amitmurthy

@yuyichao yuyichao force-pushed the yyc/tests/isolate branch from ee25e0f to f8fa519 Compare July 15, 2016 15:48
@tkelman
Copy link
Contributor

tkelman commented Jul 15, 2016

Was it @dhoegh who implemented the dict key completion? Pretty recently iirc.

@yuyichao
Copy link
Contributor Author

It looks reasonably easy to fix. I'll try later.

@dhoegh
Copy link
Contributor

dhoegh commented Jul 15, 2016

Nope, but a sensible guess. It was @bjarthur in #17017.

@tkelman
Copy link
Contributor

tkelman commented Jul 15, 2016

sorry, should have actually checked blame!

@yuyichao yuyichao force-pushed the yyc/tests/isolate branch from f8fa519 to 7ddfc23 Compare July 16, 2016 00:50
@yuyichao
Copy link
Contributor Author

Dict completion fixed. Rebased to the first commit so it can be merged independently if necessary.

@kshyatt kshyatt added the testsystem The unit testing framework and Test stdlib label Jul 16, 2016
@amitmurthy
Copy link
Contributor

I think it is important to actually have the example test run with a remote worker. That test has also uncovered an as yet open issue - #16091

@yuyichao yuyichao force-pushed the yyc/tests/isolate branch from 7ddfc23 to aca82a6 Compare July 16, 2016 12:23
test_dict = Dict("abc"=>1, "abcd"=>10, :bar=>2, :bar2=>9, Base=>3, contains=>4, `ls`=>5,
66=>7, 67=>8, ("q",3)=>11, "α"=>12, :α=>13)
s="test_dict[\"ab"
s="CompletionFoo.test_dict[\"ab"
Copy link
Contributor

Choose a reason for hiding this comment

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

if this doesn't take too long to run, should we make it a loop over both the CompletionFoo.test_dict version and the no-module test_dict version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same applies to almost all other tests in the same file though (this was basically the only one that uses a non-qualified name in the replcompletion test before). It's mainly to avoid polluting the namespace but making the name slightly longer could also be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

this was basically the only one that uses a non-qualified name in the replcompletion test before

Then shouldn't this be kept for the sake of test coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I think it makes sense to add it back with a longer name but no it won't help test coverage since this one uses a completely different code path that isn't shared with anything else.

Copy link
Contributor

@tkelman tkelman Jul 16, 2016

Choose a reason for hiding this comment

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

module qualified vs not qualified by a module. it seems like you're no longer testing the unqualified version? nevermind having it as a function works great

@yuyichao yuyichao force-pushed the yyc/tests/isolate branch from aca82a6 to a8f2cc7 Compare July 16, 2016 13:44
@tkelman
Copy link
Contributor

tkelman commented Jul 16, 2016

I'd like to do the first commit before branching (it's a bugfix) but hold off on the second until after.

@yuyichao
Copy link
Contributor Author

I'd like to do the first commit before branching (it's a bugfix) but hold off on the second until after.

Sure. FWIW the kwarg error one is probably more important but there isn't a test for it without the second commit now. I'll split this two out into a separate PR shortly.

@yuyichao yuyichao mentioned this pull request Jul 16, 2016
resp = remotecall_fetch(t -> runtests(t), p, test)
# FIXME: `remote_fetch` doesn't work when
# a new module is define.
resp = remotecall_fetch(t -> runtests(t, t != "examples"), p, test)
Copy link
Contributor

Choose a reason for hiding this comment

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

One option is to split examples.jl and move the clustermanager and dictchannel tests to parallel_exec.jl. With an appropriate comment in examples.jl explaining the rationale for the move.

@yuyichao yuyichao force-pushed the yyc/tests/isolate branch 2 times, most recently from df8b820 to 685603a Compare July 29, 2016 14:45
@yuyichao
Copy link
Contributor Author

Still waiting on branching and the appveyor failure is the git network issue...

I switch to normal module now since anonymous modules are going to be kept alive by the types defined in it anyway....

@@ -117,6 +117,8 @@ end
@test gc_enable(true)

# test methodswith
# `methodwith` relies on exported symbols
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, methodswith ?

@yuyichao yuyichao force-pushed the yyc/tests/isolate branch 13 times, most recently from 956da74 to 13aa7a4 Compare November 11, 2016 01:08
@yuyichao
Copy link
Contributor Author

Comment addressed local tests passed and CI re-enabled. This should be ready to go.

I've also tested locally that turning off isolation still pass tests and running all the tests on a single process also works without additional warnings.

All the test files can be run with both mode now. A few things that needs to be run on Main are explicitly evald in Main (e.g. the REPL, the serializer and the TestHelper module). I've also removed a few modules in the dates tests since they are not needed anymore.

So that names in different tests won't conflict.
@yuyichao
Copy link
Contributor Author

Failure was #19132 rebased and will merge when CI passed.

@yuyichao yuyichao merged commit 735554d into master Nov 14, 2016
@yuyichao yuyichao deleted the yyc/tests/isolate branch November 14, 2016 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants