WIP: Convert to Lib tests #373

guygastineau · 2019-08-06T02:47:38Z

This is not intended for merging at all, but rather it is intended to share ideas about how to convert our tests to library testing. That being said, if commits only ever involve 1 exercise, then we can easily cherry pick conversions where we agree, so they can be merged in their own PR.

For now, I just did two-fer initial PoC and triangle since it is a bit more relevant.

@glennj I played with the :: you used for name spacing in forth, but I could not get it to work for triangle. It kept returning an error status. Anyway, I will keep investigating that, but any investigation you feel like doing would be appreciated.

I noticed that nounset absolutely may not be used in the implementation files for lib testing. sourceing the file into bats, then pollutes the bats environment. Since bats relies on variables being unset sometimes it will not work. We can probably use something like bash -c "source triangle; equilateral 2 2 2" to keep from polluting the bats environment. We should make sure we discuss this ;)

closes #366

Reviewer Resources:

Track Policies

guygastineau · 2019-08-06T02:55:35Z

As I said over in #366 I wont force push to this, so feel free to clone and we can all work together on it.

I think local work on any given exercise should be rebased before pushing back up here to make cherry picking easier.

@kotp @glennj @IsaacG Let me know if you guys would prefer having this branch belong to our exercism repo outright. It might be nice if we could all just have our own version of this branch through our personal clones. Anyway, just let me know ;)

guygastineau · 2019-08-06T02:59:58Z

travis_fold:start:before_script
�[0Ktravis_time:start:1ae0802b
�[0K$ bin/fetch_configlet

gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
travis_time:end:1ae0802b:start=1565059734245936845,finish=1565060043499524003,duration=309253587158
�[0K�[31;1mThe command "bin/fetch_configlet" failed and exited with 2 during .�[0m

Your build has been stopped.

~~I have seen this happen before. Was it here? I think it is happening, because the files are .xz, but tar wants to treat them like gzip format due to an extension.~~

EDIT:

I remember now that I just needed to rerun the travis tests, because sometimes stuff fails for no reason 😟

IsaacG · 2019-08-06T04:43:29Z

exercises/triangle/example.sh

-    echo "false"
-  fi
-
+  [[ $1 -eq 0 ]] && echo 'true' || echo 'false'


(( $1 == 0 )) && ...

You are very right about that.

At least I upgraded it from [] 😂

IsaacG · 2019-08-06T04:46:12Z

exercises/triangle/triangle_test.sh

 # Test returns true if the triangle is equilateral

 @test "all sides are equal, equilateral" {
  #[[ $BATS_RUN_SKIPPED == true  ]] || skip
-  run bash triangle.sh equilateral 2 2 2
+  run equilateral 2 2 2


I'm a fan of exporting a single main or triangle function that takes a triangle-type as a first arg.
Thoughts?

Given that it is a library I think main here is not right, but I could be convinced that a managing function has its place for namespacing.

This also depends on how our other endeavors into namespacing fare.

If you have some ideas for this exercise please feel free to make a PR to this branch on my fork.

If you would prefer I am interested in making a new branch on the base repo for getting through this ordeal.

I want to know other contributor opinions first though.

Or you can make your own PR for triangle.

I think this comment thread is getting to the heart of how we want to manage name-spacing in our bash libs.

do we want any? i.e. let source just dump library functionality into the "global" scope

use a top-level function with library functions accessed as sub-commands, as suggested here

use a function prefix, like I did in the forth example

Does anyone know of any common idioms or best practice guidelines?

I think the world of bash library modules is all cowboys. We should see if Gnu org has any specification.

I learned that functions may have . in their names too.

The only problem I have is that my syntax highlighters get confused.

So, I did some more tests.
Double colon (::) looks nice, but I have also read that it is not guaranteed to work with older versions of bash. I am of the opinion that there is absolutely no reason to be using bash older than v4.x, but historically we have not wanted to make Mac users upgrade bash against their will.

I am not against setting v4.2 as a requirement for the track. Hell, I am on v5.0.7, but I would not make this requirement all on my own.

If we decide against enforcing a minimum Bash version, then I see no other way to namespace the functions than by using an module access function.

In order for this not to confuse students it should be present in the stub. We would need to decide if we write the whole thing, so they don't have to worry about it or not.

Then we are left with the decision about how to name the parts it calls. Good naming convention would then make the former API functions internal and they should be preceded by a _. I think this will get ugly very quickly, and it is not really exposing students to a common pattern in the shell world. Furthermore, it doesn't do much to help with possible collisions in the world.

While I think that :: is ideal (even if a bit non-standard) I am feeling less comfortable with the interface function pattern. I honestly think that normally lib sourcing is the way to go here.

If it makes us feel any better. Golang's tests are in the same packagespace as the code, so all of the functions are called without namespacing.

Also, our namespacing ideas were never going to solve issue of set options colliisions with bats' internal requirements. To do that we would need to execute the run commands in a sub-shell. This might work, but it needs to be tested.

@issacg I guess we were typing at the same time 🤣

It seems we both came to the same conclusion (although I am very verbose as usual).

Looking around at other languages like Python and Ruby it is very evident tha no one else is worrying about this (but in their languages those functions will be namespaced once they are imported somewhere). That just isn't the way of life here 😄

Note bash 3 (or, at least 3.2.57) supports space::func just fine. While we may not want to enforce bash 4+, I think it's fair to expect bash 3 or newer.

Thanks for checking.

guygastineau · 2019-08-06T05:04:48Z

Additionally, if these are bash libraries (they are not posix compliant) then they should have .bash as their suffix.

kotp · 2019-08-06T07:34:41Z

The invalid and wontfix labels are added just to remind not to merge.

guygastineau · 2019-08-06T12:47:43Z

Thank @kotp those labels are a good reminder.

Do you have an opinion on whether or not this project should be switched to an architecture wherein it is a direct branch of exercism/bash?

kotp · 2019-08-06T16:55:46Z

I think it is fine where it is, until it isn't. We can clone it down and work with it locally, as it is.

glennj · 2019-08-14T19:33:11Z

FYI I have not noticed any problems with function names containing :: in bash 3.2 on the mac

guygastineau · 2019-08-14T19:40:06Z

So, is it possible to get new count of our positions regarding namespaces?
The only thing that still concerns me about using :: is that it bars the solution from being posix compatible if a student wishes to do so, but that is not a big deal in my opinion.

Still my vote would be for no namespaces, since students are unlikely to encounter that so much in wild (although it does exist).

guygastineau · 2019-08-14T19:49:25Z

Tally for namespacing:

Contributors	With Function	With `::`	None
@guygastineau	❌	❌	✔️
@glennj	❌	✔️	❌
@IsaacG	❌	✔️	❌
@kotp	❌	✅	❌

glennj · 2019-08-14T19:54:51Z

So, is it possible to get new count of our positions regarding namespaces?
The only thing that still concerns me about using :: is that it bars the solution from being posix compatible if a student wishes to do so, but that is not a big deal in my opinion.

Since this is a bash-specific track and bash is not POSIX compatible, IMO don't care.

Still my vote would be for no namespaces, since students are unlikely to encounter that so much in wild (although it does exist).

This is like import library vs from library import * in python. My vote is yes, but I'll abide by majority.

@guygastineau what's "with Function" mean? Is that capitalizing the library function names?

IsaacG · 2019-08-14T20:33:25Z

I'll vote for yes namespace.

guygastineau · 2019-08-14T20:36:50Z

@IsaacG I assume you mean with :: from our earlier conversations?

guygastineau · 2019-08-14T20:38:26Z

@glennj I meant using a function that takes subcommands to act as the entry point for the library.

IsaacG · 2019-08-14T20:40:24Z

Yes, I'm in favor of using the :: for function namespaces.

glennj · 2019-08-14T21:08:42Z

ref #376

kotp · 2019-08-15T21:40:43Z

I feel like this is the Bash track, and so by definition not sh and therefore not required to be POSIX compliant. sh is a specification, and Bash is a superset of sh. I do agree that we should be doing the .bash extension, instead. And there should be a clear explanation for that in the introductory material handed to students.

It may be interesting to have a sh track as well, where strict compliance is expected, but that isn't this track. In recent times, I have noticed:

file $(which sh)
/bin/sh: symbolic link to dash

This seems to happen on Debian (and at least some derivatives). OpenBSD seems to use something a little more Korn-y.

So the practice of "portability" using !#/bin/sh is not an issue here, but could also be mentioned.

Instead, as this is the Bash track, we should use #!/usr/bin/env bash or at least specifically #!/bin/bash and let the program itself fail if Bash isn't available. Again, the focus being "This is the Bash track".

Just some thoughts as it regards my "vote" in Tally for Namespacing.

glennj · 2019-08-16T00:01:23Z

... I do agree that we should be doing the .bash extension, instead.

How is that going to affect the CI scripts? I guess we'll have to throw a little extra logic into bin/validate_exercises

kotp · 2019-08-16T00:32:27Z

... I do agree that we should be doing the .bash extension, instead.

How is that going to affect the CI scripts? I guess we'll have to throw a little extra logic into bin/validate_exercises

No direct answer to this, but no (long term) logic. All should be .bash in my opinion.

MichaelDimmitt · 2021-08-17T16:21:33Z

Really good discussion here 🔥

another thing worth noting,
diffing is easy to attain in bats

by changing

@test "a name given" {
  [[ $BATS_RUN_SKIPPED == true  ]] || skip
  run two_fer Alice
  [[ $status -eq 0 ]]
  [[ $output == "One for Alice, one for me." ]]
}

printDiff(){
  echo output;
  echo "$output";
  echo ;
  echo expected;
  echo "$expected;
}

@test "a name given" {
  [[ $BATS_RUN_SKIPPED == true  ]] || skip
  run two_fer Alice
  expected=$(cat <<-END
One for Alice, one for me.
END
  )
  printDiff
  [[ $status -eq 0 ]]
  [[ "$output" ==  "$expected" ]]
}

ErikSchierboom · 2021-08-17T18:24:10Z

And as a side note: the test runner should also be updated. It might even be necessary to support both formats to not break old exercises. Did we decide how to handle such a situation @iHiD ?

IsaacG · 2022-03-23T02:18:56Z

This PR has seen no activity in 6 months. Is it still being used or should it be closed out?

kotp · 2022-03-23T02:39:03Z

Leave it be. It is draft and still has a place.

guygastineau added 2 commits August 5, 2019 22:34

two-fer: Convert to lib testing

205c932

triangle: lib tests without namespacing

acc1f86

guygastineau changed the title ~~WIP: Lib tests~~ WIP: Convert to Lib tests Aug 6, 2019

guygastineau mentioned this pull request Aug 6, 2019

Switch Tests to Library Tests #366

Closed

guygastineau added discussion enhancement labels Aug 6, 2019

IsaacG reviewed Aug 6, 2019

View reviewed changes

kotp added invalid wontfix labels Aug 6, 2019

glennj mentioned this pull request Aug 14, 2019

Questions arise while implementing list-ops exercise as library implementation #375

Closed

Base automatically changed from master to main January 28, 2021 19:16

MichaelDimmitt mentioned this pull request Aug 17, 2021

Add diffing support to bash tests #531

Closed

MichaelDimmitt approved these changes Aug 17, 2021

View reviewed changes

kotp marked this pull request as draft December 7, 2021 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Convert to Lib tests #373

WIP: Convert to Lib tests #373

guygastineau commented Aug 6, 2019 •

edited

Loading

guygastineau commented Aug 6, 2019

guygastineau commented Aug 6, 2019 •

edited

Loading

IsaacG Aug 6, 2019

guygastineau Aug 6, 2019

IsaacG Aug 6, 2019

guygastineau Aug 6, 2019 •

edited by kotp

Loading

guygastineau Aug 6, 2019

glennj Aug 6, 2019 •

edited by kotp

Loading

guygastineau Aug 6, 2019

guygastineau Aug 8, 2019

guygastineau Aug 8, 2019

guygastineau Aug 8, 2019

IsaacG Aug 8, 2019

guygastineau Aug 8, 2019

guygastineau commented Aug 6, 2019 •

edited

Loading

kotp commented Aug 6, 2019

guygastineau commented Aug 6, 2019

kotp commented Aug 6, 2019

glennj commented Aug 14, 2019

guygastineau commented Aug 14, 2019

guygastineau commented Aug 14, 2019 •

edited by kotp

Loading

glennj commented Aug 14, 2019 •

edited

Loading

IsaacG commented Aug 14, 2019

guygastineau commented Aug 14, 2019

guygastineau commented Aug 14, 2019

IsaacG commented Aug 14, 2019

glennj commented Aug 14, 2019

kotp commented Aug 15, 2019

glennj commented Aug 16, 2019

kotp commented Aug 16, 2019

MichaelDimmitt commented Aug 17, 2021

ErikSchierboom commented Aug 17, 2021

IsaacG commented Mar 23, 2022

kotp commented Mar 23, 2022

WIP: Convert to Lib tests #373

Are you sure you want to change the base?

WIP: Convert to Lib tests #373

Conversation

guygastineau commented Aug 6, 2019 • edited Loading

guygastineau commented Aug 6, 2019

guygastineau commented Aug 6, 2019 • edited Loading

EDIT:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guygastineau Aug 6, 2019 • edited by kotp Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennj Aug 6, 2019 • edited by kotp Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guygastineau commented Aug 6, 2019 • edited Loading

kotp commented Aug 6, 2019

guygastineau commented Aug 6, 2019

kotp commented Aug 6, 2019

glennj commented Aug 14, 2019

guygastineau commented Aug 14, 2019

guygastineau commented Aug 14, 2019 • edited by kotp Loading

glennj commented Aug 14, 2019 • edited Loading

IsaacG commented Aug 14, 2019

guygastineau commented Aug 14, 2019

guygastineau commented Aug 14, 2019

IsaacG commented Aug 14, 2019

glennj commented Aug 14, 2019

kotp commented Aug 15, 2019

glennj commented Aug 16, 2019

kotp commented Aug 16, 2019

MichaelDimmitt commented Aug 17, 2021

ErikSchierboom commented Aug 17, 2021

IsaacG commented Mar 23, 2022

kotp commented Mar 23, 2022

guygastineau commented Aug 6, 2019 •

edited

Loading

guygastineau commented Aug 6, 2019 •

edited

Loading

guygastineau Aug 6, 2019 •

edited by kotp

Loading

glennj Aug 6, 2019 •

edited by kotp

Loading

guygastineau commented Aug 6, 2019 •

edited

Loading

guygastineau commented Aug 14, 2019 •

edited by kotp

Loading

glennj commented Aug 14, 2019 •

edited

Loading