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

feat(assertApi): enable agile assertions development across Camunda BPM versions #56

Merged
merged 1 commit into from
Nov 25, 2015

Conversation

martinschimak
Copy link
Contributor

Camunda BPM Assert still grows, becomes more mature ... but development slowed down. Why is that? Well, it's not lack of dedication, but I admit I felt a bit blocked recently, first without knowing conciously. I think I know now. A day of being sick in the bed brought me some time and enlightenment. 😄

Camunda BPM Assert will continue to remain community driven. This means:

  1. The assertions which run against e.g. Camunda BPM 7.0 are still far from being "feature complete" when comparing it with the many possibilities of Camunda BPM's 7.0 API. Of course that's ok, people will add or at least request what they need and would like to use.
  2. Camunda BPM's latest and greatest features (e.g. 7.2 and 7.4 bring a lot of them in the areas of CMMN and DMN) bring a lot of developer motivation to contribute new assertions, too, as e.g. pending pull request Cmmn assertions #52 prooves.

There is a certain tension in between 1 and 2. In an "ideal" world ...

  • ... highly motivated contributors to Camunda BPM Assert should be able to work against the most recent API without asking or feeling stuck with an old API...
  • ... but the releases of Camunda BPM Assert should be able to support at least the most recent and supported Camunda BPM versions as well...
  • ... and contributing and maintaining Camunda BPM Assert must remain really simple (which excludes several branches as overkill) but also safe (which e.g. means that a new assertion dealing with 7.2 API should not accidentally use a 7.4 API without actually needing it)

I came up with the following relatively unconventional solution provided here as pull request to foster community feedback. But I believe for Camunda BPM Assert's 'nature' and requirements the following approach would work really well:

  1. We continue to maintain just one simple master branch, but from now on it always compiles and builds against Camunda BPM's latest "RELEASE" (which includes alpha releases).

  2. When implementing assertions (or little static helpers), we transparently document and assert for the user that the code needs at least a specific API version:

    /**
    * @Since Camunda BPM 7.2
    */
    public StageAssert isActive() {
      assertApi("7.2");
      // ...
    }

    (Find the implementation of assertApi() here: a3066f7)

  3. [NOTE: section edited at Nov 16, some of the discussion below is obsolete because of this edit] When testing assertions (or little static helpers), we assume (with an implementation based on the JUnit Assumptions API) that the test of course also needs the requested Camunda BPM API:

    public void testStageAssertIsActive() {
      assumeApi("7.2");
      // ...
    }

With that, explicitly requested test executions against "old" Camunda BPM API versions are still possible via commandline properties, which is important for CI/Jenkins testing all those old versions. Such a build then automatically IGNORES those tests which need a newer Camunda BPM API. This is the intended effect of JUnit Assumptions raising a AssumptionViolatedException. This way, by simply testing the master against all Camunda BPM versions, the CI always makes sure that all assertions and helpers work down to the minimum Camunda API they claim to need.

I would love to get some feedback on this - but remember I am sick in the bed, so be honest, yes, but also nice and patient with me! 😄

@jangalinski
Copy link
Contributor

Hi Martin, I hope you are feeling better now. I just wanted to "ping" this to show that it got attention. But honestly I didn't get all of the description above, I will have to check out and analyse the code. That will take a while. Stay tuned. Jan

@jangalinski
Copy link
Contributor

Seems like I dont get this ... how exactly would this approach help us solve #53 ? We found that the 7.0 api cannot answer the assertion correctly so we switch to 7.2 and add "includeAssigned" to the query. What does that mean then? That hasCandidateGroup() gets an assertApi("7.2")? I cannot use it with 7.0/7.1 then anymore, right?

@jangalinski
Copy link
Contributor

An alternative solution suggestion:

  • we switch versioning to the camunda version, so it is clear which assert version relates to which camunda version.
  • each version is represented by a branch of that version
  • master always points to latest (including alphas)
  • "official" support by you (and the core team) is limited to the versions that camunda supports in ee (7.2 and up today), meaning we will fix issues and extend functionality and merge them to higher versions. Once support runs out, community members are free to file PRs for the outdated versions of course.
  • New features and fixes are added to the lowest possible branch and propagated up the tree to master.

Of course this will lead to some branches and merges, but every branch itself has a clear, repeating structure. Normally we would have to keep our eye on 3 of them: master for latest development (say 7.5-alpha) and two supported ee versions (7.4, 7.3). Motivation to contribute new features like DMN/CMMN goes up, because its just pushing to master. And maintanence is focussed and contained with a clear lifecycle announcement.

For the issues at hand that would mean: #53 can be fixed easily by useing the newer API in the 7.2 branch (and merge up). #52 can be supported in the 7.3 branch and up. And it would not be such a big issue since we would have had them in the master early on.

Imo this is simplier and cleaner ...

@martinschimak
Copy link
Contributor Author

Hi @jangalinski - many thx for pinging and thinking through this together with me! The alternative approach you describe is in my mind the "right" and kind of "default" way to deal with such things from a normal version control perspective. It is basically the approach my subconscious mind wanted to avoid 😄 when subconsciously contemplating about simpler ways which are normally marked as "to be avoided" but maybe good enough (or even better!) for this simple project. Well, I don't say I exclude the git approach, but would like to collect arguments... when comparing alternatives (and maybe there are others?) I think about how easy and fast it is to (learn how to)

  1. contribute - e.g write a pull request?
  2. maintain - e.g merge pull requests? ... upwards in between refactored or even reformatted branches?
  3. release - e.g. one or several branches - we need Core Team Member Support for that...

Another thing is that the solution should not put any peer/community pressure on me or anybody in the interested team to contribute here without actually needing it or having fun with just doing it. Bringing the version numbers close to Camunda BPM releases could in my mind bring quite a lot of such subconscious pressure...

I will let my subconsciousness work on the answers and maybe answer to myself later this week. Any further comments or opinions are very much appreciated!!

@martinschimak
Copy link
Contributor Author

I think I know now an even simpler solution. Instead of point 3) of my description above (putting tests into different modules), it should be possible to simple leave them where they are but ignore them in case they need a new api, but are executed against an older api on Jenkins. One could then e.g. write assumeApi("7.3") at the beginning of such a test method. This could be based on Junits Assumptions mechanism.

@jangalinski
Copy link
Contributor

How would your solution address the problem we have with #53? the assertion itself is there in 7.0, but we have to use an alternative implementation to fix a bug with later versions. How would marking the method as "requires api version X" help?

@martinschimak
Copy link
Contributor Author

Well, internally I already implemented the mechanism with a method

boolean supportsApi(String api) { ... }

Then

void assertApi(String api) { ... } 

uses that and throws an AssertionError in case it finds the api unsupported, as well could the mentioned

void assumeApi(String api) { ... }

use a similar mechanism. This would be the general way to go and keep the project (contribution, maintenance, release) really simple... For seldom bugs like #53, one could decide to use supportsApi("7.2") as an internal switch for an implementation like the one proposed by your pull request, e.g. when instantianting the problematic class. Which is ugly, but could be removed again, as soon as the support period for such a case ends.

@martinschimak
Copy link
Contributor Author

I edited the pull request description text above (point 3 of the proposal) after having changed the proposed implementation to use a assumeApi() call for test implementations.

@zambrovski
Copy link
Contributor

Hi Martin,

this is a interesting approach, but I think it is too artificial. You are building something like a module system / SPI with simple dependencies. You MAY do this and I think it is possible to implement it without errors and side effects, but this is definitely something I would never call simple. Most FOSS developers know the branching model of Git - in fact BECAUSE people understand this model, the development of open-source projects is so fast.

I vote AGAINST black-magic versioning inside the codebase, but FOR explicit versioning of the artifact inside the POM. Following the argumentation of @jangalinski I think it is a good idea to sync versions from the library FOR Camunda BPM to the version OF Camunda BPM. This is clear and simple to everyone, even if it produces a minimal overhead in taking care of branches (a skill that is well understood and developed by many developers). The important question for me is when to create and release those branches and versions. In fact, if Camunda BPM goes to 7.5 there is no need (or pressure) to create a 7.5 branch UNTIL there is a first feature which must be implemented against (uses explicit API of) 7.5. This means that if the Engine API remains stable from 7.4 to 7.7 there is no need to shadow the versions.

Applying this pattern I would suggest the following set of rules for versioning:

  • There MIGHT be a LATEST branch following the Camunda BPM latest development. (e.g. called latest including latest API usage of non-released API in 7.4.0-alpha2, includes DMN asserts)
  • There is one STABLE branch that corresponds to the earliest Camunda BPM supported version (e.g. called 7.2.0, includes basic BPMN asserts)
  • If there exists an API difference between this earliest version and later supported versions (currently Camunda supports two versions) AND there is at least one implemented assert using this change, there exists a second STABLE branch. (e.g. called 7.3.0, includes CMMN asserts)
  • New versions and branches are created only on demand (not triggered by any Camunda BPM development). So when Camunda releases 7.4, there is no need to switch branches or move the versions UNTIL someonw contributes something relevant to 7.4... Only if then we abbandnon the 7.2 development.

Just my two cents..

Kind regards,

Simon

@martinschimak
Copy link
Contributor Author

Hi Simon, many thanks for sharing your time and brain with me!

My impression is that most of your comment relates to the part I completely removed with the additional commits I pushed that morning. There are no modularized test classes anymore. In short, the modified suggestion is that we keep everything as is, but assert inside assertions that the method sees the needed api and assume inside assertion tests that the test sees the needed api. See my edited pull request description above for more details.

By doing that, we can always simply build against the latest Camunda BPM release on developer machines and verify via CI that the assertions use the version they declare to need. I am already building the project against all versions of Camunda BPM via Cloudbees/Jenkins right now, so I don't need to change anything for that.

While I fully agree that the development community understands git branching and everything you say in this respect, I do tend to see it as unnecessary overhead for this particular project to do that. With the proposed approach, I also don't see it as necessary to stop "supporting" earlier versions of Camunda BPM (whatever that would actually mean for a community extension). I rather expect that the project will continue to work with Camunda BPM 7.0, too, for a very long time to come, and if somebody wishes to do that I don't see any harm in that as long as it does not cause work or headaches for us.

The only little "downside" of the approach would be, that those users of very old versions will over time (when using new versions of camunda-bpm-assert) see more and more assertions they cannot make any use of. When trying to use them, their tests will immediately fail and they will be informed why. I think fair enough! But the upside would be that whenever something is added which relates to very old Camunda features, and I expect that to happen for a very long time to come, they could immediately make use of it with the next release of camunda-bpm-assert. Without any need for several version specific releases, maintenance of version specific branches, upmerging of features, selecting the "right" camunda-bpm-assert version to use as a user etc.

I want to enable more agile assertions development. Having several branches will subconciously be a burden to that goal again, because people with a shortage of free time at hand (= I assume all of us) will subconciously avoid heavy refactoring and innovative approaches (or avoid to merge such pull requests), when knowing that this means that every single little pull requests to come will as a consequence not need 2 minutes to review and merge, but 30 or more.

…a BPM version

- assertApi() for minimum needed Camunda BPM version needed for assertions
- assumeApi() for minimum needed Camunda BPM version needed for assertion tests
@martinschimak
Copy link
Contributor Author

I force-pushed a squashed single commit with the (few) remaining changes into this pull request branch in order to avoid any misunderstandings about its current content. (When re-reviewing, please use a fresh local branch in order to avoid problems with this.)

@zambrovski
Copy link
Contributor

I would buy it, if you give me the answer to the question of @jangalinski regarding how your version model allows to implement the hasCandidateGroups assert in two different ways, depending on the API version. In fact it was evel from Camunda Engine to change the behavior of default query in 7.2, but we have to deal with this. So either:

  • Implement the correct assert based on 7.2 and set assumeApi("7.2")
  • Implement the wrong assert in 7.0 - which delivers false results on > 7.2
  • Implement a dirty version-specific switch choosing the implementation dependening on API version and mark it with assumeApi("7.0")
  • Go the usual way and create two versions of the artifact called 7.0 with 7.0 API implementation and 7.2 with 7.2 API implementation.

What do you think?

@zambrovski
Copy link
Contributor

Thank you Christian, this is definietly a better implementation approach. I think the approach of @martinschimak is good if no behavioural implementation changes behind the API exist. In particular case of #53 we have to deal with implementation change behind the stable API. Any suggestions?

@martinschimak
Copy link
Contributor Author

Hi @zambrovski I agree that we would have several options in case Camunda changed the behaviour behind an API - and think those options are good enough for camunda-bpm-assert. I assume that we will never need the dirty third option mentioned by you, because Camunda is very conservative in this respect (and rightly so), but we would have it as a very last option.

For #53 I voted not to see it as a bug, but rather as an opportunity to improve camunda-bpm-assert by introducing a new assertion which would be available for Camunda 7.2 upwards. See my comment there which sums up the whole issue and discussion.

@hawky-4s- Thank you for making me aware of this! We could even combine both approaches.

@martinschimak martinschimak merged commit 16c3b7e into master Nov 25, 2015
@martinschimak martinschimak deleted the feat-assertApi branch November 26, 2015 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants