NotificationsYou must be signed in to change notification settings
Fork15
Star204

DRAFT: Order based on filenames or partial path#52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Draft

Joacchim wants to merge1 commit intopytest-dev:main

base:main

Choose a base branch

fromJoacchim:file-ordering

Draft

DRAFT: Order based on filenames or partial path#52

Joacchim wants to merge1 commit intopytest-dev:mainfromJoacchim:file-ordering

Conversation

Copy link

Joacchim commentedAug 6, 2021

As discussed in#51, I've been fiddling with the module to try and see how I could fit my needs.

This is rough on the edges, and may not follow the project's development philosophy, but aims to be a first shot to be discussed and reflected upon.

DRAFT patch to be discussed

e125278

Copy link

codecov-commenter commentedAug 7, 2021•
edited
Loading

❌ Patch coverage is69.23077% with8 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.59%. Comparing base (1e873d5) to head (e125278).
⚠️ Report is 139 commits behind head on main.

Files with missing lines	Patch %	Lines
pytest_order/sorter.py	69.23%	5 Missing and 3 partials⚠️

Additional details and impacted files

@@            Coverage Diff             @@##             main      #52      +/-   ##==========================================- Coverage   97.79%   95.59%   -2.21%==========================================  Files           5        5                Lines         545      567      +22       Branches      121      129       +8     ==========================================+ Hits          533      542       +9- Misses          8       17       +9- Partials        4        8       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:

❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Member

mrbean-bremen commentedAug 7, 2021

Thanks for that! I didn't get to this today, will probably have a closer look tomorrow (bit tired today, brain not working...), but at a first glance, this looks good to me. There is not much development philosophy to this project, other than keep it maintainable, don't add unneeded bloat, and have a test coverage close to 100% - after all, it's only a small project. If you want, you can add a couple of tests, or I can do that later.
Actually the change to use node ids as labels that I made recently makes this kind of change easier. I think it is a reasonable improvement, though we have to check that it behaves as expected with different options (likeorder-scope andorder-group-scope).

Copy link

Author

Joacchim commentedAug 7, 2021

I haven't tried cleaning-up the patch or even running the tests so far, I mostly wanted to push that out for you to read first before doing anything else.

Given your feedback, I guess I'll go a bit further on the next iteration, since it seems to be a 🆗 :)

mrbean-bremen reviewed

Aug 8, 2021

View reviewed changes

pytest_order/sorter.py

		fornode_idinself.node_ids:
		ifnode_id.count("::")==2:
		path_index=node_id.index("::")
		iflabel.endswith('/')andlabelinnode_id:# directory ordering

Copy link

Member

mrbean-bremenAug 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thein check here could be ambiguous - maybenode_id.startswith(label)? Or we need more checks here...

Copy link

Author

JoacchimAug 12, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I took the approach of checking~~endswith()~~ "in" in order to match any part of a directory's path, even if not fully specified (as it allows shorter and more concise specification in the decorator parameters).

Copy link

Member

mrbean-bremenAug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, actually I did the same withendswith, which wouldn't work in your case, of course... given that it needs to end with a slash this may be ok (it would theoretically allow ambiguities, but the user would be responsible to avoid them).

pytest_order/sorter.py

		path_index=node_id.index("::")
		iflabel.endswith('/')andlabelinnode_id:# directory ordering
		items.append(self.node_ids[node_id])
		elifnode_id[:path_index].endswith(label):# end-of-filename ordering

Copy link

Member

mrbean-bremenAug 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This implies that there is only one directory ending withlabel, though it could be documented thatlabel has to be unambiguous in that respect.

Copy link

Author

JoacchimAug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I thought it would be easier to require ending the label with a/ to refer to a directory, which would incidentally avoid confusing a directory and any other kind of path/entity.