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

JBTM-1016 Run bmcheck.sh regularly to spot inconsistencies between byteman scripts and the code #333

Merged
merged 1 commit into from Jul 17, 2013

Conversation

zhfeng
Copy link
Collaborator

@zhfeng zhfeng commented Jun 7, 2013

No description provided.

<artifactId>rulecheck-maven-plugin</artifactId>
<version>${version.org.jboss.byteman}</version>
<executions>
<execution>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a default be set for this, so that we don't have to always specify the phase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, process-test-classes is the default phase.

@paulrobinson
Copy link
Contributor

You should look for more Byteman rules to check. We have them in other places than XTS recovery tests and TXBridge tests.

<goal>rulecheck</goal>
</goals>
<configuration>
<includes>**/*.txt</includes>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do all our Byteman rules have the .txt suffix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, and my suggestion is that as part of this Jira we ensure they do and it now becomes an error to use any other extension (e.g. .btm).

They may do already.

Amos, please can you advised developers of this (once merged) via the dev forum: https://community.jboss.org/en/jbosstm/dev/?view=discussions

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we allow .txt and .btm.

Ensuring no one adds rule files with other extensions could be difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was why I was thinking of standardising on a single extension (i.e I assume btm allows arbitrary extensions). If btm only allows .btm or .txt then I agree check for both. If it could be .anything then I think we should choose one and it is an error (PR review) to allow anything but that after notification via the forum.

Therefore, do we know what extensions are supported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all our byteman rules have the .txt suffix
./ArjunaJTA/jta/target/test-classes/fail2pc.txt
./ArjunaJTA/jta/target/test-classes/recovery.txt
./ArjunaCore/arjuna/target/test-classes/objectstore.txt
./ArjunaCore/arjuna/target/test-classes/reaper.txt
./ArjunaCore/arjuna/target/test-classes/recovery.txt
./txbridge/target/test-classes/scripts/txbridge-byteman-rules.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/BASubordinateCrashDuringCommitAfterSubordinateExit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/ATHeuristicRecoveryAfterDelayedCommit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/BASubordinateCrashDuringComplete.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/BASubordinateCrashDuringCommit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/ATParticipantCrashAndRecover.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/ATSubordinateCrashDuringPrepare.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/ATCrashDuringCommit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/BACrashDuringCommit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/ATCrashDuringOnePhaseCommit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/BACrashDuringOnePhaseCommit.txt
./XTS/localjunit/crash-recovery-tests/target/test-classes/scripts/ATSubordinateCrashDuringCommit.txt
./ArjunaJTS/integration/target/test-classes/fail2pc.txt
./ArjunaJTS/integration/target/test-classes/leaveorphan.txt
./ArjunaJTS/integration/target/test-classes/leave-subordinate-orphan.txt
./ArjunaJTS/integration/target/test-classes/leaverunningorphan.txt

@jbosstm-bot
Copy link

Started testing this pull request: http://172.17.131.2/job/btny-pulls-narayana/603/

@jbosstm-bot
Copy link

Tests failed: XTS: SOME TESTS failed

@zhfeng
Copy link
Collaborator Author

zhfeng commented Jun 8, 2013

XTS/localjunit/WSTX11-interop

SEVERE: Failed: com.jboss.transaction.txinterop.interop.ATTest.testAT4_1
com.arjuna.wst.TransactionRolledBackException
at com.arjuna.wst11.stub.CompletionStub.commit(CompletionStub.java:65)
at com.jboss.transaction.txinterop.interop.ATTestCase.testAT4_1(ATTestCase.java:281)
at com.jboss.transaction.txinterop.interop.ATTest.testAT4_1(ATTest.java:104)

@zhfeng
Copy link
Collaborator Author

zhfeng commented Jun 8, 2013

It looks like https://issues.jboss.org/browse/JBTM-1751

@zhfeng
Copy link
Collaborator Author

zhfeng commented Jun 8, 2013

now all our byteman scripts check OK except some warnings with ArjunaCore/arjuna/test/byteman-scripts/reaper.txt
I'm happy to merge if adinn accepts bytemanproject/byteman#19

@paulrobinson
Copy link
Contributor

Re-scheduled PR testing...

<configuration>
<includes>
<include>**/*.txt</include>
<include>**/*.btm</include>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have the default includes as "/*.txt" and "/*.btm" set in the byteman plugin. These would be overridden if the "includes" element was set as in above. I guess this is one for @adinn. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks good to me.

@jbosstm-bot
Copy link

@jbosstm-bot
Copy link

Build Byteman Failed

@jbosstm-bot
Copy link

Started testing this pull request: http://172.17.131.2/job/btny-pulls-narayana/964/

@jbosstm-bot
Copy link

Started testing this pull request: http://172.17.131.2/job/btny-pulls-narayana-windows2008/186/

@jbosstm-bot
Copy link

All tests passed - Job complete http://172.17.131.2/job/btny-pulls-narayana-windows2008/186/

@jbosstm-bot
Copy link

All tests passed - Job complete http://172.17.131.2/job/btny-pulls-narayana/964/

@zhfeng zhfeng merged commit 0e309f5 into jbosstm:master Jul 17, 2013
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