test_fixed_issues restructure #400

jacadzaca · 2022-09-09T14:40:32Z

Restructure tests inside test_fixed_issues along the lines of #374. I tried to keep tests for similar assertions in one file.

Before considering merging, please take a look at ~~#397~~ and #399

jacadzaca · 2022-09-09T15:24:22Z

Test fails because pytz.timezone('America/Sao_Paulo') is not considered the same as (UTC-03:00) Brasília, although it is ( principal city) . ZoneInfo and dateutil.tz detect that.

niccokunzmann

So, I like where the code is heading!

There is one problem that I see.... To review these conditions seem to be important:

that no test is left out
that tests that were changed correspond to the test that was there before
that nothing was added that was not tested before

Since this is a very big PR, this will take a while and any new PR to tests will create merge conflicts.

How can we tackle that?

To make sure that this is clear: I really like the code that I see!
I find it hard to review.

I see two options:

You wait until I reviewed this
- I will go through the commits to see that the changes are consistent refectorings
- I will go though each test in the original code (master) and then try to find them in the PR
You split it up:
- Creating the base (conftest.py first), wait for it to be merged
- Create a PR for tests by cherry-picking

Also a question: Is this possible to port to 4.x?

What are your thoughts on this?

niccokunzmann · 2022-09-09T17:56:41Z

src/icalendar/tests/conftest.py

+
+@pytest.fixture
+def timezones():
+    return TIMEZONES


Sometimes, elements might be changed by tests and then, we have side effects.
Personally, I would put DataSource(TIMEZONES_FOLDER, icalendar.Timezone.from_ical) into the function body.

Does that impact execution time?

I would say that we can use __getattr__ to set the attributes and then, they will only loaded when needed. What do you think?

This is how I do it:

https://github.com/niccokunzmann/python-recurring-ical-events/blob/master/test/conftest.py#L49

... not quite like I said ...

To be honest, I didn't quite grasp the piece of code you've linked when I was writing my conftest. You're right that side effects are possible. I think implementing simple lazy loading shouldn't be too hard. I can override getattr or use the @property decorator, like you've said.

niccokunzmann · 2022-09-09T17:56:57Z

src/icalendar/tests/conftest.py

+@pytest.fixture
+def calendars_folder():
+    return CALENDARS_FOLDER
+


Nice, I like this file!

niccokunzmann · 2022-09-09T17:57:37Z

src/icalendar/tests/events/issue_112_missing_tzinfo_on_exdate.ics

@@ -1,30 +1,3 @@
-BEGIN:VCALENDAR
-PRODID:-//Google Inc//Google Calendar 70.9054//EN


Why is is removed is probably in the commit...

Seems like a trip up on my side, weird that the test still passes.

niccokunzmann · 2022-09-09T17:58:16Z

src/icalendar/tests/test_api.py

+        }
+    )
+
+    assert event.to_ical() == events.issue_116_117_add_x_apple_structured.raw_ics


niccokunzmann · 2022-09-09T17:59:02Z

src/icalendar/tests/test_encoding.py

+    # Unicode characters in summary
+    'issue_64_event_with_unicode_summary',
+    # chokes on umlauts in ORGANIZER
+    'issue_101_icalendar_chokes_on_umlauts_in_organizer'


Cool. Very clear which issue is referenced. This was important to me and it seems to work nicely.

niccokunzmann · 2022-09-09T18:08:03Z

src/icalendar/tests/test_encoding.py

+    event = getattr(events, event_name)
+    assert event.to_ical() == event.raw_ics
+
+@pytest.mark.parametrize('timezone', [


I would like to change this and add a parametrized fixture that takes the time zone string as an argument, so we make sure we have one place in which we list all the different time zone implementations.

https://github.com/niccokunzmann/python-recurring-ical-events/blob/7f6aeed4729b97981c22ef0f481029cca225ae76/test/conftest.py#L120

In conftest:

# for parametrizing fixtures, see https://docs.pytest.org/en/latest/fixture.html#parametrizing-fixtures @pytest.fixture(params=[pytz.timezone, tz.gettz, zoneinfo.ZoneInfo], scope="module") def TZ(request): return request.param()

Here:

def test_parses_event_with_non_ascii_tzid_issue_237(events, TZ): ... expected = datetime.datetime(2017, 5, 11, 13, 30, tzinfo=TZ('America/Sao_Paulo'))

This way, we can add a timezone implementation easily in one place for all tests.

We can use this fixture in the future, if we were to specify a timezone parameter for the parsing of the ics files.

What do you think?

https://github.com/collective/icalendar/runs/8273351016?check_suite_focus=true#step:6:58 would be solved by passing a parameter to the parser of which implementation to use.

Should that be in the commit with the test or a separate one?

niccokunzmann · 2022-09-09T18:11:35Z

src/icalendar/tests/test_issue_104_mark_events_broken.py

+    '''Issue #104 - line parsing error in a VCALENDAR
+    (which doesn't have ignore_exceptions). Should raise an exception.
+    '''
+    calendar_path = os.path.join(calendars_folder, 'issue_104_broken_calendar.ics')


This would use the fixture.

niccokunzmann · 2022-09-09T18:11:56Z

src/icalendar/tests/test_issue_335_identify_UTC.py

+except ModuleNotFoundError:
+    from backports import zoneinfo
+
+from icalendar import Event


WIP, this is empty.

I think I forgot to remove this file, when I moved test 335 to test_parsing. I will remove it

niccokunzmann · 2022-09-09T18:12:56Z

src/icalendar/tests/test_parsing.py

+    zoneinfo.ZoneInfo('UTC'),
+    pytz.timezone('UTC'),
+    tz.UTC,
+    tz.gettz('UTC')])


calls for a general fixture in conftest: utc

niccokunzmann · 2022-09-09T18:14:24Z

src/icalendar/tests/test_parsing.py

+    recur = events.issue_70_rrule_causes_attribute_error.decoded('RRULE')
+    assert isinstance(recur, vRecur)
+    assert recur.to_ical() == b'FREQ=WEEKLY;UNTIL=20070619T225959;INTERVAL=1'
+


I really like the test structure, the different fixtures and the way that there are a few assertions always at the end.

jacadzaca · 2022-09-09T18:54:23Z

@niccokunzmann Now that I think about it, it really is a big PR. Tomorrow-ish, I will implement the suggestion you've made and cherry pick the PR into few smaller ones (option 2). Thanks :)

angatha · 2022-09-27T18:09:47Z

@jacadzaca nice work that you provide to this package 👍

jacadzaca added 18 commits September 9, 2022 13:08

restructure test_issue_53

17f788d

restructure test_issue_58

9d02c2c

restructure test_issue_64

6eb317e

restructure test_issue_82

f45842e

restructure test_issue_101

744deca

restructure test_issue_104

a0658f5

restructure test_issue_112

fbd7f72

restructure test_issue_116

e1b5266

move test_index_error_issue to test_parsing

e52d320

move test_issue_335 to test_parsing

c704ff9

refactor test_issue_345

eb34bc7

refactor test_issue_142 and test_issue_143

60d5781

refactor test_issue_157

86b1df6

refactor test_issue_178

7d2bfed

refactor test_issue_168

b4aa4b8

refactor test_issue_100 and test_issue_184

738078d

refactor test_issue_55

39955b5

refactor test_issue_70

84b2c7b

jacadzaca force-pushed the test_restructure1 branch 2 times, most recently from fc78387 to 93f47ef Compare September 9, 2022 15:23

niccokunzmann requested changes Sep 9, 2022

View reviewed changes

This was referenced Sep 20, 2022

refactor test_issue_345 #415

Merged

refactor test_issue_142 and test_issue_143 #416

Merged

refactor test_issue_178 #417

Merged

refactor issue 273

577d233

jacadzaca force-pushed the test_restructure1 branch from 93f47ef to 577d233 Compare September 20, 2022 12:39

jacadzaca mentioned this pull request Sep 20, 2022

refactor test_issue_237 #418

Merged

jacadzaca closed this Sep 20, 2022

jacadzaca mentioned this pull request Oct 7, 2022

restructure test_issue_82 #440

Merged

niccokunzmann mentioned this pull request Oct 7, 2022

Making icalendar.from_ical independent of pytz and usable with all timezone implementations #441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_fixed_issues restructure #400

test_fixed_issues restructure #400

jacadzaca commented Sep 9, 2022 •

edited

Loading

jacadzaca commented Sep 9, 2022

niccokunzmann left a comment

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

jacadzaca Sep 9, 2022 •

edited

Loading

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

jacadzaca Sep 9, 2022

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

jacadzaca Sep 9, 2022

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

jacadzaca Sep 9, 2022

niccokunzmann Sep 9, 2022

niccokunzmann Sep 9, 2022

jacadzaca commented Sep 9, 2022 •

edited

Loading

angatha commented Sep 27, 2022

		@@ -1,30 +1,3 @@
		BEGIN:VCALENDAR
		PRODID:-//Google Inc//Google Calendar 70.9054//EN

test_fixed_issues restructure #400

test_fixed_issues restructure #400

Conversation

jacadzaca commented Sep 9, 2022 • edited Loading

jacadzaca commented Sep 9, 2022

niccokunzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacadzaca Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacadzaca commented Sep 9, 2022 • edited Loading

angatha commented Sep 27, 2022

jacadzaca commented Sep 9, 2022 •

edited

Loading

jacadzaca Sep 9, 2022 •

edited

Loading

jacadzaca commented Sep 9, 2022 •

edited

Loading