Skip to content

Add enabled flag to Item#57

Merged
reinecke merged 11 commits intoOpenTimelineIO:mainfrom
KarthikRIyer:enabled
Apr 5, 2022
Merged

Add enabled flag to Item#57
reinecke merged 11 commits intoOpenTimelineIO:mainfrom
KarthikRIyer:enabled

Conversation

@KarthikRIyer
Copy link
Copy Markdown
Contributor

@KarthikRIyer KarthikRIyer commented Feb 12, 2022

Closes #56

This PR adds:

  • The enabled flag to Item.
  • Proposes to add and further write tests in the BDD (Behaviour Driven Development) format.

The idea of BDD testing is to write tests in a simple user/business friendly manner. The tests become a documentation in themselves. For example:

If we want to write a simple test to see if disabling an Item works...

The BDD specification would be like:

given: "An item is created"
when: "Item is disabled"
then: "Item does not contribute to the composition"

And the code would be like:

given: "An item is created"
def item = new Item()
verify {
    item.enabled() == true
}

when: "Item is disabled"
item.setEnabled(false)

then: "Item does not contribute to the composition"
verifyAll {
        item.isEnabled() == false
}

The PR has the same tests, both in the unit testing format and BDD format, for comparison.

I've used spock, a groovy BDD testing framework. It might be a lot of effort in the beginning but once you get used to it, it becomes simpler and clearer. It'll also be helpful to community members new to the project. The current tests might not be very clear to them.
Spock also has an extension spock-reports that generates nice HTML test reports. But I haven't been able to get it working with the bindings yet.

Any thoughts on taking this forward? Or should we continue writing unit test as we do right now?

If we do take it forward, we might also want to adopt this for the python bindings. There are plenty of frameworks available, but in particular pytest-bdd is compatible with pytest. (Framework comparison)

@reinecke @meshula @jminor @ssteinbach

@KarthikRIyer
Copy link
Copy Markdown
Contributor Author

KarthikRIyer commented Feb 17, 2022

Spock reports works now!

image
image

@KarthikRIyer
Copy link
Copy Markdown
Contributor Author

Hi @reinecke, could you please take a look at this too

Copy link
Copy Markdown
Member

@reinecke reinecke left a comment

Choose a reason for hiding this comment

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

In terms of isEnabled - one small question on deprecating the old constructor.

On BDD testing:

I support migrating to spock tests for our Java implementation, however I would prefer that we do that its own separate PR.

In terms of BDD testing in python, I think that choice is entangled with some language-specific details (like which frameworks we use, what the current best practice in the community is), so we should have that discussion on an issue in the main repo.

Comment thread src/main/java/io/opentimeline/opentimelineio/Item.java
@KarthikRIyer
Copy link
Copy Markdown
Contributor Author

@reinecke this PR only has the wrapper for the new flag now. I think this is good to merge.

Copy link
Copy Markdown
Member

@reinecke reinecke left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

@reinecke reinecke merged commit 441865e into OpenTimelineIO:main Apr 5, 2022
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.

Add enabled flag to Item

2 participants