Skip attribute on MultiNodeFact does not work

[Aaronontheweb]

Still ran the tests I wanted to skip :\

[schatekar]

@Aaronontheweb looking at the source for MultiNodeFact below.

This is returning null from skip if the test is run using MultiNodeTestRunner or NodeTestRunner. I am not sure if XUnit treats a null Skip value as test not to be skipped or not but if I assume that then clearly any test run using MultiNodeTestRunner would never be skipped, isn't it?

    public class MultiNodeFactAttribute : FactAttribute
    {
        public static Lazy<bool> ExecutedByMultiNodeRunner =
            new Lazy<bool>(() =>
            {
                var args = Environment.GetCommandLineArgs();
                if (args.Length == 0) return false;
                var firstArg = args[0];
                return firstArg.Contains("Akka.MultiNodeTestRunner") 
                    || firstArg.Contains("Akka.NodeTestRunner");
            });

        public override string Skip
        {
            get
            {
                return ExecutedByMultiNodeRunner.Value
                    ? null
                    : "Must be executed by multi-node test runner";
            }
            set { base.Skip = value; }
        }
    }

[cpx86]

Just started looking at this as well :) @schatekar, a null value means test should be run, and non-null means it should be skipped.

[schatekar]

I think I know how to fix this. If you are not fixing it I can take a crack
at it in few hours.
On 6 Aug 2015 18:54, "Christian Palmstierna" notifications@github.com
wrote:

Just started looking at this as well :) @schatekar
https://github.com/schatekar, a null value means test should be run,
and non-null means it should be skipped.

—
Reply to this email directly or view it on GitHub
#1088 (comment)
.

[cpx86]

It's all yours :)

[Aaronontheweb]

You guys are awesome :)

[schatekar]

This is more involved than I had imagined. I would give it another try over the weekend.

[Aaronontheweb]

This is more involved than I had imagined. I would give it another try over the weekend.

I know that feeling well, my friend.

[barambani]

I gave a try to this as well, I guess because is less intimidating than the others :), it looks like that removing the null is not enough. I could get the runner to skip the multinode facts checking the attribute in the OnMessage of the Akka.MultiNodeTestRunner.Discovery. It looks like that the discovery process completely ignores the Skip (overridden or not).
I'm not sure though if this is the best solution so I would like to discuss it with you guys. I will create a pull request and will be very happy to get any suggestions from you.

[schatekar]

@barambani I did not get a message that you have worked on the issue as well. I just went ahead and submitted my fix without knowing that you too have submitted a fix.

[barambani]

@schatekar no problem, I will close mine.

[schatekar]

@barambani Don't close yours just yet. I would leave it to the project leads to accept the best PR

[barambani]

@schatekar I looked at your solution and passes all the tests, also brings the skip reason up to the top level to log it. I like it more than mine as well. Mine was a late night hack.