Deleting default Component, Milestone, Version or Enum doesn't clear default value

[Ryan J Ollos]

Deleting the default Component, Milestone, Version or Enum leaves the [ticket] default_ entry in trac.ini having a non-existent resource id for a value. The entry could be cleared when the resource is deleted. For example, if [ticket] default_milestone = milestone1 and milestone1 is deleted, the entry could be cleared, [ticket] default_milestone = .

[walty <walty8@…>]

I suppose Priority, Resolution, Severity, Ticket Type and Version would do the same thing?

btw, I am not sure where is the Enum..?

[walty <walty8@…>]

Replying to walty <walty8@…>:

I suppose Priority, Resolution, Severity, Ticket Type and Version would do the same thing?

btw, I am not sure where is the Enum..?

pls ignore this comment, I just found the inheritance between the four and Enum class in the code.

[walty <walty8@…>]

A patch was just uploaded, please advise on the best practice/sample of the unit tests of this one.

btw, I found a minor bug during testing the patch, clearing default of Enum values (e.g. priority) would crash, I have fixed that as well (at the end of the patch). I am not sure if I should exclude that part from the patch.

thanks.

[Ryan J Ollos]

Replying to walty <walty8@…>:

btw, I found a minor bug during testing the patch, clearing default of Enum values (e.g. priority) would crash, I have fixed that as well (at the end of the patch). I am not sure if I should exclude that part from the patch.

Thanks for catching that. Fixed in [14267] (see comment:11:ticket:11765).

[walty <walty8@…>]

Replying to rjollos:

Replying to walty <walty8@…>:

btw, I found a minor bug during testing the patch, clearing default of Enum values (e.g. priority) would crash, I have fixed that as well (at the end of the patch). I am not sure if I should exclude that part from the patch.

Thanks for catching that. Fixed in [14267] (see comment:11:ticket:11765).

You are welcome, I think I should remove that part from the patch then.

btw, I guess I better add the functional tests at the same place (tests/functional/main.py) for the new code?

[Ryan J Ollos]

Replying to walty <walty8@…>:

btw, I guess I better add the functional tests at the same place (tests/functional/main.py) for the new code?

You could write unit tests since only the logic in admin.py needs to be tested. The functional tests are slow so we tend to only use them when template code needs to be tested. I added an admin test module in [14271:14272].

[walty <walty8@…>]

hi, the new patch includes the unit tests for the components (clean up default value of trac.ini), pls advise if this is fine. I would work on other modules if this patch is OK.

two further questions here:

1. I found that the unit test file of admin is very simple now, so for my patch, do I need to write the unit tests for my patch only, or I better write the unit tests of others as well (e.g. add/remove priorities).

2. Since the unit tests of these admin panels are very alike, do u mind if I use some sort of inheritance inside the test suite so the code could be more effectively written? or it's actually a bad practice in the field of unit test?

thanks.

[Ryan J Ollos]

Replying to walty <walty8@…>:

hi, the new patch includes the unit tests for the components (clean up default value of trac.ini), pls advise if this is fine.

Ideally each test case will test just one thing. component1 and component2 are added when the EnvironmentStub is created with default_data=True so you can work with those components to avoid needing to add a component to setup your test case.

1. I found that the unit test file of admin is very simple now, so for my patch, do I need to write the unit tests for my patch only, or I better write the unit tests of others as well (e.g. add/remove priorities).

I think you can just focus on adding coverage for your patch, unless you potential for breaking some other behavior and want to add coverage just to be sure.

2. Since the unit tests of these admin panels are very alike, do u mind if I use some sort of inheritance inside the test suite so the code could be more effectively written? or it's actually a bad practice in the field of unit test?

I think it's fine to use inheritance, especially when creating a mixin class. You could find many examples in our test suite in which we've used a base class to avoid test duplication.

[walty <walty8@…>]

hi,

I have uploaded a new patch, with base class and mixin class added into the unit test. My main reference is tracopt/versioncontrol/git/tests/git_fs.py. Please see if this version is fine.

thanks

[Ryan J Ollos]

I reviewed the patch and will push revised changes soon. No need to update the patch, but a few comments for future consideration:

• Consider reviewing ​PEP-0008 and using a linter on your changes.
• Ideally tests should be a bit more isolated from one another, each testing only a single behavior.
• There were a few copy/paste errors in the test that didn't lead to actual failures, but made the tests confusing to read.

Those are just small things and the changes look good.

[walty <walty8@…>]

Replying to Ryan J Ollos:

I reviewed the patch and will push revised changes soon. No need to update the patch, but a few comments for future consideration:

• Consider reviewing ​PEP-0008 and using a linter on your changes.
• Ideally tests should be a bit more isolated from one another, each testing only a single behavior.
• There were a few copy/paste errors in the test that didn't lead to actual failures, but made the tests confusing to read.

Those are just small things and the changes look good.

thanks a lot for your review and comments!

[Ryan J Ollos]

Committed to trunk in [14292].