dgw / yourls-dont-track-admins

A plugin for YOURLS (the self-hosted URL shortener) that disables tracking clicks on shortened URLs for logged-in users.
https://github.com/dgw/yourls-dont-track-admins
GNU General Public License v3.0
33 stars 7 forks source link

Unit tests ! Automation ! Success ! #10

Closed ozh closed 2 years ago

ozh commented 2 years ago

I used your plugin as a use case to build https://github.com/YOURLS/YOURLS-test-suite-for-plugins (inspired from recent discussions about people unable to log in with your plugin activated)

This PR brings :

I'll tell you my friend : this is truly a new dawn for YOURLS plugins :-)

ozh commented 2 years ago

To see how tests run, see in my fork : https://github.com/ozh/yourls-dont-track-admins/actions

dgw commented 2 years ago

The new testing framework is 🤯 of course! I got so caught up in the new YAML that I forgot to say that… 😅

ozh commented 2 years ago

That's the beauty of squash commits, one PR makes one feature and one commit :)

Aside from this and your personal preference for full commit history in PR, 1) I don't know how to do it now that it's been pushed in Github :) and 2) I wouldn't know what commit to keep or to squash since all the commits are basically tweaks or fixes for the initial one :)

dgw commented 2 years ago

1) I don't know how to do it now that it's been pushed in Github

If you have a local shell with Git it's not too difficult. I have a lot of practice with git rebase, the tool you'd need, and there are add-ons you can install to make rebasing even simpler.

2) I wouldn't know what commit to keep or to squash since all the commits are basically tweaks or fixes for the initial one

The first three commits (before 240dd7b7cba99c80afd7212f60e9c61ad79f0be1) are unrelated, actually. They're left over from #9, which you apparently opened using your master as HEAD, and then syncing your master branch with mine. All three could be dropped.

In sequence form (like you'd see in the rebase editor), I would do this:

pick 240dd7b Introduce unit tests
pick d9e0679 Automate tests
fixup 5e76f5d s/localhost/127.0.0.1/
fixup a9e7164 Don't create table
fixup 4d10804 With no password...
fixup 81fb6be Debug: list files
fixup 05d4302 Shallow clone of YOURLS/Ytsfp
pick 32666fc Run tests when there's a new YOURLS release
fixup 340d7d6 New YOURLS release

That leaves the logical steps and evolution in the history:

But it leaves out the intermediate steps along the way where the automated testing didn't work correctly yet. That's my biggest litmus test for deciding what to squash: If a commit is somehow broken, a later commit that fixes the brokenness should be combined with it so no point in the history exists in a broken state.

IMO rebasing is useful on both sides of a code review. Before review, rewriting history into logical steps makes the patch easier for the reviewer to digest. After review, rewriting history again gives you a nice clean branch to merge back in with any tweaks quietly applied to the relevant logical steps—as I said, to avoid leaving any broken points in history.

I admit, squash-and-merge is faster. It also takes away a lot of the joy I find in looking back at a project's history, though. Squashing stuff reduces any development done off of the default branch down to, essentially, only the merges. For lack of a better word, it's boring; sometimes I just like to open gitk or similar and see the branch lines weaving in and out of each other, and you don't get anything interesting to look at with a squash-and-merge history. The graph ends up almost entirely linear.

(Did I intend on writing a miniature philosophical dissertation about version control history? Absolutely not, but it happened.)

Back to the point: If you don't want to muck with the whole interactive-rebasing thing, I can massage the PR history myself, then give you a command to run just so your changes will still have the nice green "Verified" badge. Or not. I just don't want ugly yellow "Unverified" badges to appear if you turned on strict signing… 😅

ozh commented 2 years ago

Yeah please go ahead and "massage the PR history" :)

ozh commented 2 years ago

I've rebased my local repo and now it's a mess apparently ...

image

ozh commented 2 years ago

OK apparently I just needed to push --force instead of just push. And now there are commits appearing prior to my commits...

ozh commented 2 years ago

@dgw I'm leaving you at this stage now ¯_(ツ)_/¯

dgw commented 2 years ago

Definitely didn't use the correct base somewhere along the line. I will try to fix it, but if it turns out not to be simple you'll have to wait until tomorrow. It's already past 4am here. 😅

dgw commented 2 years ago

Actually it's impossible for me to undo on my own, as the old commits don't exist in my local repo. You'll have to reset back to the previous state—try git push --force origin tests:5d31d886f53d8a17077791a92c081699cb9ba590 (that was the HEAD commit before you overwrote things with the cleaned-up history).

ozh commented 2 years ago

That just creates another branch with the same 6 commits

dgw commented 2 years ago

Because 4am Syndrome struck me… Reverse it: git push --force origin 5d31d886f53d8a17077791a92c081699cb9ba590:tests

dgw commented 2 years ago

This is more like it. Cleanup incoming!

image

ozh commented 2 years ago

\o/ Thanks :) (way too much hassle and work for the benefit, sticking to squash and merge on my side ;)

dgw commented 2 years ago

I dunno about hassle. Took me approx. 30 seconds once I could git fetch ozh the correct refs (after your attempt, they were dangling/orphaned and Git doesn't allow directly fetching orphan commits). But the speed probably comes with practice… I've done this a lot over the years.

FWIW, where you went wrong on the rebase (I think) is by trying to use HEAD~8 and such instead of just git rebase -i master. Maybe that made things more complicated? And/or you didn't know to drop that first commit, because I forgot about it when creating that sample rebase sequence from before.

Anyway, thanks for trying it out. You can git push origin :5d31d886f53d8a17077791a92c081699cb9ba590 to remove the stupid extra branch I had you create by mistake. Definitely time for 🛌 now lol

ozh commented 2 years ago

Another side effect I see is that now my fork and your repo are not synced anymore, I would probably to have to git reset hard something I guess

dgw commented 2 years ago

You were already out of sync because of using your master as a PR head branch and then doing a merge (19edc8194e1de02b96d8501aba9603318435897d) instead of overwriting. Pretty easy to fix, though. You can add this repo as a remote and fetch it then reset --hard dgw/master as you said—or you can just delete the fork and create a fresh one in the future if you want to make another PR.

GitHub's lack of decent tools for syncing forks to upstream is kind of a disgrace, but that's a whole other rant I shouldn't bore you with unless asked. 😁