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

ISPN-4323 ConditionalOperationsConcurrentWriteSkewTest fails randomly #2804

Closed
wants to merge 1 commit into from

Conversation

wburns
Copy link
Member

@wburns wburns commented Aug 18, 2014

  • Changed 500 invocation tests to stress as they can last for minutes
  • Moved more simple test to separate file and still functional

https://issues.jboss.org/browse/ISPN-4323

@wburns
Copy link
Member Author

wburns commented Aug 18, 2014

I moved the various tests to the stress package as they can be lead to quite a few false positives while other tests are running. If you don't like that let me know.

@danberindei
Copy link
Member

@wburns I'd rather keep the "simple" tests in the original files and create ConditionalOperationsConcurrent*StressTest for the ones with 500 iterations.

@wburns
Copy link
Member Author

wburns commented Aug 19, 2014

Sure, the other thing I was quite sure on is that only the write skew has a simple test implementation currently. Do we want to have simple tests for the others as well? Maybe I should log a JIRA?

@danberindei
Copy link
Member

Yeah, l think we want simple tests for the others as well. Another option would be to duplicate the tests we have now, only the functional test would have less threads/iterations and the stress test would have more threads/iterations.

@wburns
Copy link
Member Author

wburns commented Aug 19, 2014

Sure I can do it that way as well.

@danberindei
Copy link
Member

I almost forgot we should probably do the same with ConditionalOperationsTest.

@wburns
Copy link
Member Author

wburns commented Aug 19, 2014

Yeah I was planning on getting to all the classes that extend ConditionalOperationsTest including it.

@danberindei
Copy link
Member

Oops, I meant ConcurrentOperationsTest

@wburns
Copy link
Member Author

wburns commented Aug 19, 2014

:)

@wburns
Copy link
Member Author

wburns commented Aug 19, 2014

Actually you were are right I was thinking of ConditionalOperationsConcurrentTest, thanks.

@wburns
Copy link
Member Author

wburns commented Aug 19, 2014

Updated with new test classes.

@pruivo
Copy link
Member

pruivo commented Aug 20, 2014

@wburns I would change the author and since from the new classes :)

testOnCaches(caches, new ConditionalRemoveOperation(false));
}

public void testPutIfAbsent() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

missing @Override

* Changed 500 invocation tests to stress as they can last for minutes
* Moved more simple test to separate file and still functional
@wburns
Copy link
Member Author

wburns commented Aug 21, 2014

Updated comments and rebased. Also regarding the author tags and version: I added my author tag to the new classes but left the original one, since these new classes are just using the previous one essentially and I added 7.0 version to the new ones.

*
* @author Sanne Grinovero <sanne@infinispan.org> (C) 2012 Red Hat Inc.
* @author William Burns
* @see java.util.concurrent.ConcurrentMap#replace(Object, Object, Object)
Copy link
Member

Choose a reason for hiding this comment

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

why see the replace?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is on the original Test class when I copied it over, TBH I had removed it for a sec but kept it just because the other one had it.

https://github.com/infinispan/infinispan/blob/master/core/src/test/java/org/infinispan/api/ConditionalOperationsConcurrentTest.java#L37

I am fine with removing if you want. I also thought of adding the other methods as well, but I didn't really mind either way, heh.

Copy link
Member

Choose a reason for hiding this comment

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

well, leave it... I'm already started to run the test suite

@pruivo
Copy link
Member

pruivo commented Aug 21, 2014

integrated! thanks @wburns !

@pruivo pruivo closed this Aug 21, 2014
@wburns wburns deleted the ISPN-4323 branch November 4, 2014 15:20
@rvansa
Copy link
Member

rvansa commented Oct 19, 2016

Funny thing, this PR effectively disabled those tests. In ConditionalOperationsConcurrentTest, generateValidMoves is called before operations is initialized, therefore no operations are actually executed. But we have a green testsuite, yikes! :)

@rvansa
Copy link
Member

rvansa commented Oct 19, 2016

#4625 should re-enable the test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants