pytest-dev / pytest-bdd

BDD library for the py.test runner
https://pytest-bdd.readthedocs.io/en/latest/
MIT License
1.3k stars 219 forks source link

improve parser performance #623

Closed dcendents closed 1 year ago

dcendents commented 1 year ago

Hi,

Using py-spy to check where to improve the performance of my tests, I noticed a lot of time was spent in the Step name property. Just by caching the value instead of recalculating it every time I gain ~15%. Same gain when I run the tests in parallel

Sequence:

Parallel:

It is a very minor change, hope it can be merged.

Thanks

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 :tada:

Comparison is base (73c2393) 95.40% compared to head (c52a5fa) 95.45%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #623 +/- ## ========================================== + Coverage 95.40% 95.45% +0.05% ========================================== Files 49 49 Lines 1763 1783 +20 Branches 160 195 +35 ========================================== + Hits 1682 1702 +20 Misses 53 53 Partials 28 28 ``` | [Impacted Files](https://app.codecov.io/gh/pytest-dev/pytest-bdd/pull/623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytest-dev) | Coverage Δ | | |---|---|---| | [src/pytest\_bdd/parser.py](https://app.codecov.io/gh/pytest-dev/pytest-bdd/pull/623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytest-dev#diff-c3JjL3B5dGVzdF9iZGQvcGFyc2VyLnB5) | `98.68% <100.00%> (+0.05%)` | :arrow_up: | | [tests/steps/test\_common.py](https://app.codecov.io/gh/pytest-dev/pytest-bdd/pull/623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytest-dev#diff-dGVzdHMvc3RlcHMvdGVzdF9jb21tb24ucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

dcendents commented 1 year ago

thanks for the review, I have modified the code to use cached_property. However there is no setter on a cached_property so I still had to create a new property to keep name as is.

Performance is the same

Let me know what you think

dcendents commented 1 year ago

Hi @youtux ,

Actually you were right we can del self.full_name but we do need to validate the key is in __dict__ first, which I was not doing the first time around and caused the error I mentionned.

Anyway that showed my modifications were lacking proper testing so I added a unit test as well to validate the cache behavior. I really tried to use a mock to wrap the step object and spy on the full_name method (property) but I could not make it work, so I hope you are ok with the way I tested it.

Changing values directly on the step object to show the cache value is still returned (which should never happen in production code). Changing values using properties show the cache is invalidated.

Cheers