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:46
@aheejin
Copy link
Member

aheejin commented Jul 7, 2021

Maybe it's too late for this at this point given that another similar PR has been merged, but I wonder moving all files to test/lit/passes directory in one PR and modifying them in place in another PR would allow us not losing the "moving" history of files.. Currently git recognize some files as moves but others as just new files.

@tlively
Copy link
Member Author

tlively commented Jul 7, 2021

I think it would be a little strange to have tests in the lit directory that are not yet lit tests. Also, I don't know of a good way to have lit run some of the .wast files in a directory and have the custom check script run the other .wast files in the same directory. Is git detecting moved files valuable? Since the files keep their names, it should be easy to manually track files back through these commits.

@aheejin
Copy link
Member

aheejin commented Jul 7, 2021

I think it would be a little strange to have tests in the lit directory that are not yet lit tests. Also, I don't know of a good way to have lit run some of the .wast files in a directory and have the custom check script run the other .wast files in the same directory.

If we are to go this way, I guess we have to ignore the CI for those PRs.

Is git detecting moved files valuable? Since the files keep their names, it should be easy to manually track files back through these commits.

It's generally better to preserve git history, that's all. You can use git log --follow to follow the history of the file including moves.

What do you think? @kripken

@tlively tlively force-pushed the port-tests-3-asyncify branch from e72c983 to 5f6bebe Compare July 8, 2021 16:02
Base automatically changed from port-tests-3-asyncify to main July 8, 2021 18:15
@kripken
Copy link
Member

kripken commented Jul 9, 2021

(Sorry, I wrote in the other PR first. As I said there, I personally don't see a need to do extra work for --follow to work, but if it would be useful to anyone else on the team then 👍 from me.)

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

If both of you find it not very useful, then maybe it is not.. I'm OK either way.

@tlively tlively merged commit d22bc1c into main Jul 13, 2021
@tlively tlively deleted the port-tests-4-flatten branch July 13, 2021 00:37
@tlively
Copy link
Member Author

tlively commented Jul 13, 2021

Thanks, @aheejin! If it were simpler to do the two phase approach I think it might be worth it, but given the extra complexity I don't think it's worth it, so I'll land these PRs without it.

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