scrapinghub / web-poet

Web scraping Page Objects core library
https://web-poet.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
93 stars 15 forks source link

Implement Stats #175

Closed Gallaecio closed 1 year ago

codecov[bot] commented 1 year ago

Codecov Report

Merging #175 (5d7bdc5) into master (1095f57) will increase coverage by 0.03%. The diff coverage is 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #175 +/- ## ========================================== + Coverage 98.40% 98.44% +0.03% ========================================== Files 29 30 +1 Lines 1383 1414 +31 ========================================== + Hits 1361 1392 +31 Misses 22 22 ``` | [Files Changed](https://app.codecov.io/gh/scrapinghub/web-poet/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapinghub) | Coverage Δ | | |---|---|---| | [web\_poet/\_\_init\_\_.py](https://app.codecov.io/gh/scrapinghub/web-poet/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapinghub#diff-d2ViX3BvZXQvX19pbml0X18ucHk=) | `100.00% <ø> (ø)` | | | [web\_poet/page\_inputs/\_\_init\_\_.py](https://app.codecov.io/gh/scrapinghub/web-poet/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapinghub#diff-d2ViX3BvZXQvcGFnZV9pbnB1dHMvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [web\_poet/page\_inputs/stats.py](https://app.codecov.io/gh/scrapinghub/web-poet/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapinghub#diff-d2ViX3BvZXQvcGFnZV9pbnB1dHMvc3RhdHMucHk=) | `100.00% <100.00%> (ø)` | | | [web\_poet/serialization/functions.py](https://app.codecov.io/gh/scrapinghub/web-poet/pull/175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapinghub#diff-d2ViX3BvZXQvc2VyaWFsaXphdGlvbi9mdW5jdGlvbnMucHk=) | `98.92% <100.00%> (+0.06%)` | :arrow_up: |
Gallaecio commented 1 year ago

OK, Stats is abstract now. What do we do about serialization for test fixtures? Is that something for frameworks to handle on their own?

kmike commented 1 year ago

OK, Stats is abstract now. What do we do about serialization for test fixtures? Is that something for frameworks to handle on their own?

I'm not sure. Stats are write-only, so it seems the Stats object itself shouldn't affect the output of the page object in any way, unless there is some bug/exception.

We need to ensure though that the stats class is passed to the page object during the test, and we can't rely on a framework like scrapy-poet to do so, because the tests shouldn't depend on it.

Maybe create something like DummyStats, and pass it during the test running? So, to serialize do nothing (or mark as something empty), and to deserialize return DummyStats?

@wRAR any feedback on this?

Gallaecio commented 1 year ago

I think we may want to test stat writing, i.e. make sure the same stats are written by the page object as when the text fixture was generated (well, the final stats, replacing inc();inc() with inc(2) should not break a test I think).

Maybe we can implement DummyStats with the pre-ABS implementation, and recover the serialization code we had for that here?

kmike commented 1 year ago

I think we may want to test stat writing

We can do it, but it'd be quite a big shift from what the current framework tests. You'd need to have some special support for this particular dependency in the testing framework, and generate a new test item.

I'd consider it a separate feature, and implement it only after the usefulnesss is clear. E.g. is it good or bad that your tests start to fail when you add some new stats to the page object? What kind of bugs do we try to prevent?

Gallaecio commented 1 year ago

Maybe create something like DummyStats, and pass it during the test running? So, to serialize do nothing (or mark as something empty), and to deserialize return DummyStats?

I have tried 2 approaches. Serializing as {} and not serializing at all. I definitely prefer the latter from a user perspective, but I have some doubts about my specific implementation.

kmike commented 1 year ago

Looks great, thanks @Gallaecio!