iTop
iTop copied to clipboard
Combodo
⚡ Don't walk over portals twice
Hello, Dev team reviewed this : as this is a change with very few benefits we are asking that a test covers modified code to ensure no regression is introduced.
Hi @piRGoif, can you point me to the current test of GetAllowedPortals so I can update it accordingly?
Hello @Hipska , Pierre is off for the next 2 weeks.
Regarding your question, there is no unit test on this yet, but I'm sure your alreaday knew that 😁 Several PRs introduced regressions in the past, so now we more and more ask for unit tests to be part of the PRs to minimize that risk, especially as it helps understanding the use cases.
As you can see in the contribution guide, we ordinary only accept PRs for bug fixes or translations; this kind of optimization isn't in the scope. Of course we make exceptions when PRs make sense and seem well-coded but keep in mind that we are a very small team with limited bandwidth and a huge roadmap ahead. This is why we can't afford spending too much time testing / reproducing a feature / optimization that we don't seem to need at the moment.
So, it doesn't make sense, or isn't well coded then? 😛
Anyway, why I put the ball back in your camp is, I don't see how I could write a sensible test for this specific part of the code.. It's easy to write a test that doesn't make sense, but I don't think you would be happy with that.
So, it doesn't make sense, or isn't well coded then? 😛
It is actually because it makes sense and is well coded that we are willing to make an exception.
Anyway, why I put the ball back in your camp is, I don't see how I could write a sensible test for this specific part of the code.. It's easy to write a test that doesn't make sense, but I don't think you would be happy with that.
You can make a unit test for the UserRights::GetAllowedPortals() method to prove that the outcome will remain the same after the alteration from this PR. You can mock PortalDispatcherData::GetData() to return different sets of data.
Okay thanks, but that will take some time until I have really nothing else to do 😄 (I'm really not into testing yet, I know I should, but since I'm not a developer by profession, I need to find spare time to learn myself the skills)