Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented Jul 7, 2021

No description provided.

@tlively tlively requested review from aheejin and kripken July 7, 2021 17:41
Base automatically changed from port-tests-2-Ox to main July 7, 2021 18:28
@kripken
Copy link
Member

kripken commented Jul 8, 2021

78 files changed is quite a lot... how about limiting to single digits?

@tlively
Copy link
Member Author

tlively commented Jul 8, 2021

Ugh, GitHub's automatic retargeting of PRs seems totally broken :( This diff contains changes that have already landed on main. Will manually rebase the branch to fix it.

@tlively tlively force-pushed the port-tests-3-asyncify branch from e72c983 to 5f6bebe Compare July 8, 2021 16:02
@tlively
Copy link
Member Author

tlively commented Jul 8, 2021

Ok, after fixing that there are 48 files changed, which means 16 tests. That's not quite single digits; do you want me to break it down further? For reference, there are 117 tests left to be automatically ported after these.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Ideally I'd prefer even smaller PRs, but this is manageable.

@tlively tlively merged commit 397a06e into main Jul 8, 2021
@tlively tlively deleted the port-tests-3-asyncify branch July 8, 2021 18:15
@aheejin
Copy link
Member

aheejin commented Jul 8, 2021

So... we decided not to do #3971 (comment) ..? I thought at least talking about it was worth it...

@aheejin
Copy link
Member

aheejin commented Jul 8, 2021

Anyway it'll be already too late that multiple PRs have been merged. Nevermind.

@tlively
Copy link
Member Author

tlively commented Jul 8, 2021

Sorry, @aheejin, I should have waited for that conversation to be resolved before merging more PRs. If we think it would be useful to port the tests in two phases, we can still do that for most of them. I don't think I would personally find it useful, but I also didn't know about git log --follow until you mentioned it. Do you or @kripken look through the histories of the individual test files often enough that this would be worth it?

@aheejin
Copy link
Member

aheejin commented Jul 8, 2021

I don't use git log --follow much, but I use git blame a lot (and Github UI for git blame). I think git blame history will be preserved for moved files, but I'm not very sure if Github UI supports it smoothly. I mostly use git blame for things like, if I don't understand what a snippet of code does, I would track the commit that code was created and look at the test that accompanies it. So yeah, it is trackable without preserved history with maybe a little more effort, so I am not saying it is very critical, but this repo has years of history so if it is easy enough to preserve them I would do that. Also these PRs are special, so I think it is justified to ignore CI for those special PRs.

But that was my thought, and if people don't think it's useful enough, I am OK with the current PRs. I just wanted to at least discuss a little before picking a direction, that's all.

@aheejin
Copy link
Member

aheejin commented Jul 8, 2021

Also I didn't think about this when I was doing #3923 either myself...

@kripken
Copy link
Member

kripken commented Jul 9, 2021

I also didn't know about --follow, interesting...

I don't feel strongly myself. I think git history matters a lot for code, but maybe less for tests. At least for myself. If it's useful for others than I'd support the two-stage approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants