Skip to content

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Nov 10, 2022

The tip of this branch fixes #5546. The rest is a series of NFC refactors that lead up to the fix, and/or were inspired by looking at that logic.

Fixes: #5546

@chrisbobbe
Copy link
Contributor

Thanks! One thought I had while reading this: are we still applying this workaround correctly in tools/test:

# Careful! `set -e` doesn't do everything you'd think it does. In
# fact, we don't get its benefit in any of the `run_foo` functions.
#
# This is because it has an effect only when it can exit the whole shell.
# (Its full name is `set -o errexit`, and it means "exit" literally.)  See:
#   https://www.gnu.org/software/bash/manual/bash.html#The-Set-Builtin
#
# When one test suite fails, we want to go on to run the other suites, so
# we use `||` to prevent the whole script from exiting there, and that
# defeats `set -e`.
#
# For now our workaround is to put `|| return` in the `run_foo` just
# after each nontrivial command that isn't the final command in the
# function.
set -e

I think we are, from looking through tools/test at the end of the branch. As long as you've made the changes with this workaround in mind, please merge at will. 🙂

@gnprice
Copy link
Member Author

gnprice commented Nov 17, 2022

Good question.

I wasn't actually thinking about that when I wrote these changes. But rereading them now, I think none of them affect it. Merging; thanks for the review!

We have other scripts that print other kinds of information.
This one is about information from Git.
This is NFC because none of our scripts currently pass any arguments
at all to `tools/info changed-files`.
This lets Git do more of the work, and should be a bit more efficient.
This makes the name a bit shorter, before we start calling it
in more places.
This makes existing callsites a bit longer, but lets us simplify
some other commands by switching to `tools/git changed-files`.
This makes all the suites' runners parallel to each other in shape.
This better juxtaposes this function's logic with other things
its caller is doing... which brings into view a bug where they
don't agree.
@gnprice gnprice merged commit 6ca7122 into zulip:main Nov 17, 2022
@gnprice gnprice deleted the pr-test-script branch November 17, 2022 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI skips Prettier on .js.flow type definitions
2 participants