Ensure functionality of spacefish on the 3.0 release of fish #128

matchai · 2018-12-28T18:04:50Z

Description

All the following changes should be backward compatible with fish 2.7.1:

Change stderr redirects to be 2> instead of ^
Resolve to physical PWD when truncating git root
Remove Appveyor and Cygwin-specific testing logic
Add Fish 2.X and Fish 3.X tests to Travis

Motivation and Context

Closes #127

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

I have tested using MacOS
I have tested using Linux

Checklist:

I have checked that no other PR duplicates mine
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have updated the tests accordingly.

I realized that we rely on it to auto-run the test script

Fish plans to deprecate caret stderr redirection in favor of POSIX-compatible redirection (fish-shell/fish-shell#4394)

matchai · 2018-12-28T20:52:47Z

I'm having a heck of a time trying to figure out what is causing this failure.
This would be a whole lot easier if the test runner would escape all backslash characters and color codes so that we can more easily debug 🤔
Edit: Oh! We totally can by looking at the travis log!

I was thinking it had to do with the new fish_ambiguous_width and fish_emoji_width variables, but messing with those locally still causes the same failures.

This reverts commit da1eb41.

matchai · 2018-12-28T23:50:44Z

After a little bit of chatting on the fish-shell gitter, this bit of advice might help us isolate the failure:

matchai · 2018-12-29T16:53:02Z

Here's the rest of the conversation, where @faho discusses the solution he found:

The failure was resolved in the end with the following PR to fishtape: jorgebucaran/fishtape#35

matchai · 2018-12-29T18:36:50Z

Hmm... 🤔
Now we've got a new failure to do with identifying the root of a git repo.
This also doesn't happen outside of tests and only seems to affect the Travis run.

Necessary due to the following update: fish-shell/fish-shell#3350

It appeared that pwd -P was only added as part of fish 3.0.0 and would not be available on fish 2.7.1

faho · 2018-12-29T20:53:11Z

functions/__sf_section_dir.fish

@@ -27,11 +27,12 @@ function __sf_section_dir -d "Display the current truncated directory"

 	set -l dir
 	set -l tmp
+	set -l resolvedPwd (pwd -P 2>/dev/null)


For easy compatibility with 2.7.0, put ; or pwd in there, because that didn't have the -P option and will error out.

Unfortunately, this causes a bug on 2.7.0 due to pwd being the logical location while the git root is a physical location.
It seems to work consistently now when using command pwd -P. Do you see any drawbacks to this approach?

Unfortunately, this causes a bug on 2.7.0 due to pwd being the logical location while the git root is a physical location.

In 2.7.0, the pwd is the physical location, always.

So pwd prints the same thing in 2.7.0, as pwd -P does in 3.0.

Using just pwd causes a bug in 3.0, not 2.7.

Just to be clear, my proposal is:

set -l resolvedPWD (pwd -P 2>/dev/null; or pwd)

which will try pwd -P, which will fail if the option isn't supported, and then it'll run pwd.

It seems to work consistently now when using command pwd -P. Do you see any drawbacks to this approach?

It's an additional fork (command pwd takes a millisecond or so on my system, hot-cache, while the builtin is unmeasurable). Plus, it's an additional external command you require.

pwd (with -P) is a POSIX requirement, but still.

Not something that's hugely important.

Excellent! Yes, you're right. It was 3.0 that caused the bug.
Thank you for the clarification. 😄

matchai · 2018-12-29T21:38:26Z

I've gone ahead and removed Appveyor and Cygwin-related testing from the project.
Appveyor has been consistently triggering false positives which are not reproducible on my local Cygwin installation, causing us to add logic specifically for Appveyor test runs.

I'll look into re-adding Cygwin (and possibly WSL) coverage with Travis in a future PR. 👍

Snuggle

It seems fishtape is having compatibility issues, heh.
https://travis-ci.org/matchai/spacefish/jobs/473447715#L149

matchai · 2018-12-29T23:47:53Z

So it seems... 🤔
It appears our tests are still running correctly, regardless. 🤷‍♂️

matchai · 2018-12-31T00:46:30Z

tests/__sf_section_user.test.fish

@@ -4,6 +4,10 @@ function setup
 	spacefish_test_setup
 end

+function teardown
+	set USER $LOGNAME


This will reset the user to be the username that initially logged in.
It seems this wasn't being reset after setting user to "ROOT".

## [1.12.4](v1.12.3...v1.12.4) (2019-01-02) ### Bug Fixes * fix bugs caused by the 3.0 release of fish ([#128](#128)) ([16539f5](16539f5))

matchai · 2019-01-02T16:01:20Z

🎉 This PR is included in version 1.12.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

faho · 2019-01-02T16:13:37Z

tests/mock.fish

@@ -18,7 +18,7 @@
 #

 function mock -a cmd -a argument -a exit_code -a executed_code -d "Mock library for fish shell testing"
-	set -l cmd_blacklist "builtin" "functions" "eval" "command"
+	set -l cmd_blacklist "builtin" "functions" "eval" "command" "argparse" "read" "set" "status" "test"


This is missing [, the alternate name for test. Though, technically, there's a few more. See https://github.com/fish-shell/fish-shell/blob/master/src/parser_keywords.cpp. So the list is

"[" and argparse begin break builtin case command continue else end exec for function if not or read return set status switch test while

(yes, [ is the only one that needs to be quoted)

This seems like a pain in the butt to maintain as things are added over time. Can fish do this for us? Is there a command to list all builtins automatically?

faho · 2019-01-09T10:37:19Z

`builtin -n`, though that lists _all_ builtins, even the ones you are allowed to override, like `cd`. Am Mi., 9. Jan. 2019, 11:17 hat Snuggle <notifications@github.com> geschrieben:

…

***@***.**** commented on this pull request. ------------------------------ In tests/mock.fish <#128 (comment)>: > @@ -18,7 +18,7 @@ # function mock -a cmd -a argument -a exit_code -a executed_code -d "Mock library for fish shell testing" - set -l cmd_blacklist "builtin" "functions" "eval" "command" + set -l cmd_blacklist "builtin" "functions" "eval" "command" "argparse" "read" "set" "status" "test" This seems like a pain in the butt to maintain as things are added over time. Can fish do this for us? Is there a command to list all builtins automatically? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE8fVx6YEIeQwYzn2FhvyVOGiJ26oMnGks5vBcGdgaJpZM4ZkF3r> .

Snuggle · 2019-02-07T09:54:29Z

@matchai Could we possibly replace this list of builtins with the above? It seems like it would be much easier to maintain.

matchai · 2019-02-07T17:40:33Z

As part of #136, the mock code is no longer in the repo.
In fish-mock, I've gone ahead and added the full list. 😄

Matan Kushner added 7 commits December 28, 2018 13:03

Add separate test runs for fish 2 and 3

1c8b6d2

Set language to node only on release

db1f47d

Set brew fish urls to be global env variables

4c5ccb4

Set language for tests to "generic"

2a212d1

Try using the "minimal" language

d209d4f

Fix typo in language

6fb63ae

Readd node language

8231ab5

I realized that we rely on it to auto-run the test script

matchai added the Type: Maintenance Non-functional changes (testing, project structure, CI, etc.) label Dec 28, 2018

Matan Kushner added 2 commits December 28, 2018 14:19

Skip brew update

bd65db3

Replace caret redirection

2884014

Fish plans to deprecate caret stderr redirection in favor of POSIX-compatible redirection (fish-shell/fish-shell#4394)

Matan Kushner added 3 commits December 28, 2018 16:40

Add new reserved words to mock

148f957

Experiment with character width

da1eb41

Revert "Experiment with character width"

3b35c52

This reverts commit da1eb41.

matchai added Help Wanted Extra attention is needed and removed Help Wanted Extra attention is needed labels Dec 29, 2018

Fix user test failure

269feb8

Matan Kushner added 2 commits December 29, 2018 13:02

Merge branch 'master' into fish-3.0

cd572f2

Fix appveyor failing to run killall

b30c613

Matan Kushner added 2 commits December 29, 2018 14:58

Use resolved symlink when truncating the git root

8da0f4c

Necessary due to the following update: fish-shell/fish-shell#3350

Use readlink -f instead of pwd -P

86e9aa4

It appeared that pwd -P was only added as part of fish 3.0.0 and would not be available on fish 2.7.1

faho reviewed Dec 29, 2018

View reviewed changes

Matan Kushner added 5 commits December 29, 2018 15:53

Use non-fish pwd

4cb4314

Set node_js to stable

63ebdf0

Clear any potentially lingering variables

82c47e8

Tidy up .travis.yml

e568c16

Update the fish version badge in README.md

d113117

Matan Kushner added 3 commits December 29, 2018 16:15

Clear variables if necessary

8105f64

Remove variables being cleared

4c6db4b

Remove appveyor coverage from project

5858c61

matchai requested a review from Snuggle December 29, 2018 21:38

matchai changed the title ~~[WIP] Ensure functionality of spacefish on the 3.0 release of fish~~ Ensure functionality of spacefish on the 3.0 release of fish Dec 29, 2018

Matan Kushner added 4 commits December 29, 2018 16:43

Remove unnecessary teardown

c18f057

Use the latest fish version when testing fish 3

7e32575

Use pwd -P and fallback on pwd

124ff2b

Add teardown step for user

e3aebc2

Snuggle reviewed Dec 29, 2018

View reviewed changes

matchai commented Dec 31, 2018

View reviewed changes

matchai merged commit 16539f5 into master Jan 2, 2019

matchai pushed a commit that referenced this pull request Jan 2, 2019

chore(release): 1.12.4 [skip ci]

569566c

## [1.12.4](v1.12.3...v1.12.4) (2019-01-02) ### Bug Fixes * fix bugs caused by the 3.0 release of fish ([#128](#128)) ([16539f5](16539f5))

matchai added the Status: Released label Jan 2, 2019

faho reviewed Jan 2, 2019

View reviewed changes

matchai deleted the fish-3.0 branch January 9, 2019 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure functionality of spacefish on the 3.0 release of fish #128

Ensure functionality of spacefish on the 3.0 release of fish #128

matchai commented Dec 28, 2018 •

edited

Loading

matchai commented Dec 28, 2018 •

edited

Loading

matchai commented Dec 28, 2018

matchai commented Dec 29, 2018

matchai commented Dec 29, 2018

faho Dec 29, 2018

matchai Dec 29, 2018

faho Dec 29, 2018

matchai Dec 29, 2018

matchai commented Dec 29, 2018

Snuggle left a comment

matchai commented Dec 29, 2018

matchai Dec 31, 2018

matchai commented Jan 2, 2019

faho Jan 2, 2019

Snuggle Jan 9, 2019

faho commented Jan 9, 2019 via email

Snuggle commented Feb 7, 2019

matchai commented Feb 7, 2019

Ensure functionality of spacefish on the 3.0 release of fish #128

Ensure functionality of spacefish on the 3.0 release of fish #128

Conversation

matchai commented Dec 28, 2018 • edited Loading

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

matchai commented Dec 28, 2018 • edited Loading

matchai commented Dec 28, 2018

matchai commented Dec 29, 2018

matchai commented Dec 29, 2018

faho Dec 29, 2018

Choose a reason for hiding this comment

matchai Dec 29, 2018

Choose a reason for hiding this comment

faho Dec 29, 2018

Choose a reason for hiding this comment

matchai Dec 29, 2018

Choose a reason for hiding this comment

matchai commented Dec 29, 2018

Snuggle left a comment

Choose a reason for hiding this comment

matchai commented Dec 29, 2018

matchai Dec 31, 2018

Choose a reason for hiding this comment

matchai commented Jan 2, 2019

faho Jan 2, 2019

Choose a reason for hiding this comment

Snuggle Jan 9, 2019

Choose a reason for hiding this comment

faho commented Jan 9, 2019 via email

Snuggle commented Feb 7, 2019

matchai commented Feb 7, 2019

matchai commented Dec 28, 2018 •

edited

Loading

matchai commented Dec 28, 2018 •

edited

Loading