feat: fix `gno test` for `_test.gno` and `_filetest.gno` files #945

harry-hov · 2023-07-04T14:46:03Z

Fix bug where gno test command doesn't recognize _test.gno/_filetest.gno file if directly passed as arg.

master:

harry-hov:hariom/gno-test-improv:

Fixes another bug where gno test avoids absolute path in arguments.

master:

harry-hov:hariom/gno-test-improv:

Note: This PR may contain some code that will be used to support future PR like #682 and PR that I will open after this to support patterns in arg.

Checklists...

Contributors Checklist

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

Maintainers Checklist

Checked that the author followed the guidelines in CONTRIBUTING.md
Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

zivkovicmilos

I've left a few comments that should be addressed before going forward, I think we can simplify the complex loops introduced

gnovm/cmd/gno/util.go

gnovm/pkg/gnomod/pkg.go

gnovm/cmd/gno/test_test.go

tbruyelle

very nice improvements

ajnavarro

Logic is more convoluted than it should be. I recommend writing down what you want to accomplish with your logic, and after that start writing the code. That will help you to organize your ideas and therefore the final code.

Ping me if you need any help or any other explanation from my comments.

gnovm/cmd/gno/util.go

gnovm/pkg/gnomod/pkg.go

gnovm/cmd/gno/util.go

gnovm/pkg/gnomod/pkg.go

harry-hov · 2023-07-12T20:56:06Z

Logic is more convoluted than it should be.

Thanks @ajnavarro for your review. I made my points in the reply. Happy to discuss more.

I recommend writing down what you want to accomplish with your logic, and after that start writing the code. That will help you to organize your ideas and therefore the final code.

Feedback taken :)

Ping me if you need any help or any other explanation from my comments.

Sure. Thanks again :)

gnovm/pkg/gnomod/pkg.go

jaekwon · 2023-07-27T16:45:39Z

gno.land/cmd/gnoland/start.go

@@ -234,7 +234,7 @@ func makeGenesisDoc(

 	for _, pkg := range nonDraftPkgs {
 		// open files in directory as MemPackage.
-		memPkg := gno.ReadMemPackage(pkg.Path(), pkg.Name())
+		memPkg := gno.ReadMemPackage(pkg.Dir, pkg.Name)


this usage of Dir is OK since you need to file location to load.

jaekwon · 2023-07-27T16:46:09Z

gnovm/cmd/gno/test.go

@@ -133,9 +134,13 @@ func execTest(cfg *testCfg, args []string, io *commands.IO) error {
 		cfg.rootDir = guessRootDir()
 	}

-	pkgPaths, err := gnoPackagesFromArgs(args)
+	paths, err := gnoPackagesFromArgs(args)


better to leave it as pkgPaths so as to differentiate from file paths.

but they can be file paths too.

e.g. paths can contain /path/to/pkg, /path/to/pkg/file_test.gno, or /path/to/pkg/z[N]_filetest.gno

Later I'm also planning to support patterns in args. e.g. gno test ./..., gno test /examples/.../pkg etc (just like go test)

paths can have pkgpath. filepath or both depends on the args

Let's do the renaming in a new PR so we can focus on names.

Suggestion:

Suggested change

paths, err := gnoPackagesFromArgs(args)

targets, err := targetsFromPatterns(args)

// targetsFromPatterns returns a list of local dirs or single files // intended to be used by gno commands such as `gno test`. func targetsFromPatterns(args []string) []target { // TODO: support './...' }

jaekwon · 2023-07-27T17:41:17Z

gnovm/cmd/gno/test.go

@@ -145,75 +150,72 @@ func execTest(cfg *testCfg, args []string, io *commands.IO) error {
 		}()
 	}

+	subPkgs, err := gnomod.SubPkgsFromPaths(paths)


we CAN have a function called "SubPkgsFromDir" (singular), but not "SubPkgsFromDirs" or "SubPkgsFromPath" or "SubPkgsFromPaths".

Based on how go list ./crypto/... ./foo ./crypto/dsa works (https://github.com/golang/go/blob/499a12009938fe2ffff90775832b9d67ca3e46b2/src/cmd/go/internal/list/list.go#L569C24-L569C35), they also use a single helper that takes a list of targets.

The main difference is that they are only supporting dirs and not single file, while we want our CLI to basically support this at the end: gno test ./crypto/... ./foo ./bar_test.gno with files too.

I think we should consider that we have:

PkgPath when onchain or when identified by gno.mod or other methods

Dir in the case a PkgPath is located in the file system (but mabe we won't need it anymore), and

Target, in my opinion target should be the only way we identify a os.Args that can be a pattern, a dir or a file.

Edit: @harry-hov found this which is better: https://github.com/golang/go/blob/master/src/cmd/go/internal/load/pkg.go#L2816; it consists of looking for packages, but creating virtual packages of a single file when a file is passed instead of a directory or pattern.

jaekwon · 2023-07-27T17:43:32Z

gnovm/cmd/gno/test.go

 		if cfg.precompile {
 			if verbose {
-				io.ErrPrintfln("=== PREC  %s", pkgPath)
+				io.ErrPrintfln("=== PREC  %s", pkg.Dir)


my main point is that here, we should use pkg.Path, which is not the same as any Dir. because "PkgPath/pkgPath/pkgpath/pkg.Path" are all the same "package path" identifier, whereas anything that ends with "Dir" is a filesystem location. They can diverge. Also a package path starts with a domain unless it is special, like a stdlibs package or something special like "main". Dir is a different beast based on the filesystem. Either absolute or relative, not something that starts with "gno.land" nor "main". It might be "." or "./something" though, or "/something". We should ideally make sure all Dirs start with a dot or slash.

I agree we should print pkg.Path instead of Dir. original var pkgPath also contains Dir. So this didn't changed anything.

Will fix this. But in another PR. (Out of scope for this PR)

jaekwon · 2023-07-27T17:45:53Z

gnovm/cmd/gno/test.go

 			}
 			precompileOpts := newPrecompileOptions(&precompileCfg{
 				output: tempdirRoot,
 			})
-			err := precompilePkg(importPath(pkgPath), precompileOpts)
+			err := precompilePkg(importPath(pkg.Dir), precompileOpts)


ditto. importPath should take pkg.Path.
we should use the type system to help here.
notice that in values.go, type PackageValue has Path as a Name type.
a package path is a Name (it is a name in its function).
a package path is a string (like all file paths and dir paths are strings).

ok I see that importPath is declared as a type importPath string.
this is confusing to me.
something is either a package path, in which case we can just use Name.
we could say type packagePath Name but I don't think it's necessary.

there can be some function that converts a package path to a file path,
but that function would need more context, more than just 1 argument.
it could be a method attached to a structure that holds more context.

Other than such a function, we shouldn't be converting package path Names to file paths or dir paths with type conversions, because nobody should be doing such a thing without calling the official function for such conversions (above).

An "import path" I don't see why we need to create such a name, because it is nothing more than package paths. So ImportPkgPaths is less confusing. Or just Imports of Names.

Any path that is not a package path, should be clearly named as such, by calling it a "file path" or a "dir path" that is a string, never a Name type. Anything like package path, pkgPath, pkg.Path, are all package paths. If you want to associate a file path or a dir path it will have to be called pkg.FilePath or pkg.DirPath.

original var pkgPath also contains Dir

Yeah, importPath is confusing. need to modify precompile related code for that. Will fix in another PR (Out of scope for this PR)

jaekwon · 2023-07-27T17:56:01Z

gnovm/cmd/gno/test.go

 			continue
 		}

-		sort.Strings(unittestFiles)
-		sort.Strings(filetestFiles)
+		sort.Strings(pkg.TestGnoFiles)


GnoTests string
GnoFiletests string
?

or TestFiles, FiletestFiles?

jaekwon · 2023-07-27T17:57:55Z

gnovm/cmd/gno/util.go

-				// cannot use path.Join or filepath.Join, because we need
-				// to ensure that ./ is the prefix to pass to go build.
-				pkg := "./" + parentDir
+				pkg := parentDir


would be better also to rename this pkgDir
since pkg is ambiguous, pkgpath or dirpath

jaekwon · 2023-07-27T17:59:40Z

gnovm/pkg/gnomod/pkg.go

@@ -5,40 +5,32 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 )

 type Pkg struct {


I think type Module would be better.
They are all modules, by virtue of having .mod files right?
This differentiates from PackageValue, and amino Package too.

I initially had the same thought.

But we don't seem to use word module anywhere.
We only use the word pkg (and pkg with state = realm)

jaekwon · 2023-07-27T18:00:01Z

gnovm/pkg/gnomod/pkg.go

-	path     string
-	draft    bool
-	requires []string
+	Dir      string   // absolute path to package dir


Dir here is fine but there should also be a PkgPath.
Module.PkgPath Name.

We already have Name string field for pkg name/path.

Maybe rename it to Path? (s/Name/Path)

jaekwon · 2023-07-27T18:01:41Z

gnovm/pkg/gnomod/pkg.go

+	Draft    bool     // whether the package is a draft
+}
+
+type SubPkg struct {


We shouldn't have anything called Sub*.
From the perspective of Go/Gno, "sub" packages are just independent packages.
We can have maybe one utility function that returns all "sub" Modules inside a dir, but that should be about it. (And btw the filetests folder once we support that, is not itself a module. For safety we should also disallow a module in a folder with that name too).

jaekwon · 2023-07-27T18:02:37Z

gnovm/pkg/gnomod/pkg.go

+	Dir        string   // absolute path to package dir
+	ImportPath string   // import path of package
+	Root       string   // Root dir containing this package, i.e dir containing gno.mod file
+	Imports    []string // imports used by this package


ImportPaths []Name // I think ImportPkgPath is just too long.
or
ImportDirs []string // but I don't think this is what we want.

We already have ImportPath string // import path of package

Don't you think ImportPaths []Name will make things more confusing?

ImportDirs []string? No. since they are not dirs

jaekwon · 2023-07-27T18:03:55Z

gnovm/pkg/gnomod/pkg.go

+
+	GnoFiles         []string // .gno source files (excluding TestGnoFiles, FiletestGnoFiles)
+	TestGnoFiles     []string // _test.gno source files
+	FiletestGnoFiles []string // _filetest.gno source files


maybe
FilePaths []string
TestFilePaths []string
FiletestFilePaths []string

BTW we might support other langauges that compile to .gno, like .gnoffee

The original thought process behind this is to have pkg in a structured/sorted/filtered way.

If we want to support another language we can easily add another field here.
e.g. PyFiles []string GnoffeeFiles []string...

This way we can apply specific operations on them accordingly.
e.g.
GnoffeeFiles []string - needs to be compiled to .gno
GnoFiles []string - vm understands it natively
TestGnoFiles []string - if you wants to run tests, these are your files

gnovm/cmd/gno/util.go

moul

Merging now, since the logic is good.

The wording concerns who were not introduced by this PR but already there, will be fixed in an upcoming PR, asap.

…ng#945) Fix bug where `gno test` command doesn't recognize `_test.gno`/`_filetest.gno` file if directly passed as arg. Fixes another bug where `gno test` avoids absolute path in arguments. Note: This PR may contain some code that will be used to support future PR like gnolang#682 and PR that I will open after this to support patterns in arg.

harry-hov self-assigned this Jul 4, 2023

github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jul 4, 2023

harry-hov changed the title ~~Improv gno test to test both pkgs and files~~ feat: improv gno test to test both pkgs and files Jul 4, 2023

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 5, 2023

harry-hov marked this pull request as ready for review July 5, 2023 04:48

harry-hov requested a review from a team as a code owner July 5, 2023 04:48

harry-hov changed the title ~~feat: improv gno test to test both pkgs and files~~ feat: improve gno test to test both pkgs and files Jul 5, 2023

zivkovicmilos requested review from zivkovicmilos, ajnavarro and thehowl July 5, 2023 10:37

zivkovicmilos reviewed Jul 7, 2023

View reviewed changes

moul reviewed Jul 10, 2023

View reviewed changes

gnovm/cmd/gno/test_test.go Outdated Show resolved Hide resolved

tbruyelle approved these changes Jul 11, 2023

View reviewed changes

harry-hov added 10 commits July 11, 2023 17:39

Export and rename Pkg fields

35d5064

Improv 'gno test' to test both pkgs and files

022d434

fix lint errors

986a7c8

Set subPkg Dir that was inadvertently omitted

56aecb4

remove ./ prefix from pkg paths

c037e22

fix gno test failing tests

07cc842

Fix makefiles and workflows

8c87cc6

Combine conditional statements

6911013

Combine conditional stmts

95a335d

reset recursiveLookup for every iteration

1e6ce13

moul mentioned this pull request Jul 11, 2023

tmp: current open PRs related to gno test, by hariom moul/sandbox#1

Open

ajnavarro requested changes Jul 12, 2023

View reviewed changes

gnovm/cmd/gno/util.go Outdated Show resolved Hide resolved

gnovm/pkg/gnomod/pkg.go Show resolved Hide resolved

gnovm/pkg/gnomod/pkg.go Show resolved Hide resolved

gnovm/cmd/gno/util.go Outdated Show resolved Hide resolved

gnovm/pkg/gnomod/pkg.go Outdated Show resolved Hide resolved

remove unnecessary newEmptySubPkg()

7cb7960

harry-hov force-pushed the hariom/gno-test-improv branch from 52ba43f to 7cb7960 Compare July 12, 2023 19:06

Remove pattern support

d96e536

harry-hov requested a review from a team July 13, 2023 17:32

ajnavarro self-requested a review July 14, 2023 14:51

Merge branch 'master' into hariom/gno-test-improv

3b729be

harry-hov mentioned this pull request Jul 23, 2023

gnomod bug demo gnolang/getting-started#18

Draft

Merge branch 'master' into hariom/gno-test-improv

038d8c5

moul approved these changes Jul 27, 2023

View reviewed changes

moul reviewed Jul 27, 2023

View reviewed changes

gnovm/pkg/gnomod/pkg.go Show resolved Hide resolved

jaekwon reviewed Jul 27, 2023

View reviewed changes

moul mentioned this pull request Jul 27, 2023

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

jaekwon reviewed Jul 27, 2023

View reviewed changes

tbruyelle reviewed Jul 31, 2023

View reviewed changes

gnovm/cmd/gno/util.go Show resolved Hide resolved

tbruyelle mentioned this pull request Aug 1, 2023

docs(gnovm): gno test help long description #1010

Merged

7 tasks

This was referenced Aug 3, 2023

Gnopher hole: Module VS Pkg VS Path VS Dir VS SubPkg VS SubModule VS SubDir VS SubPath ... #1024

Closed

Make gnomod optional everywhere #1039

Open

moul approved these changes Aug 10, 2023

View reviewed changes

Merge branch 'master' into hariom/gno-test-improv

5c0e053

ajnavarro approved these changes Aug 10, 2023

View reviewed changes

Merge branch 'master' into hariom/gno-test-improv

d5ef003

harry-hov merged commit 5240e97 into gnolang:master Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix `gno test` for `_test.gno` and `_filetest.gno` files #945

feat: fix `gno test` for `_test.gno` and `_filetest.gno` files #945

harry-hov commented Jul 4, 2023 •

edited

Loading

zivkovicmilos left a comment

tbruyelle left a comment

ajnavarro left a comment

harry-hov commented Jul 12, 2023

jaekwon Jul 27, 2023

jaekwon Jul 27, 2023

harry-hov Jul 30, 2023 •

edited

Loading

moul Aug 3, 2023

jaekwon Jul 27, 2023

moul Aug 3, 2023

jaekwon Jul 27, 2023

harry-hov Aug 10, 2023 •

edited

Loading

jaekwon Jul 27, 2023

jaekwon Jul 27, 2023

harry-hov Aug 10, 2023 •

edited

Loading

jaekwon Jul 27, 2023

jaekwon Jul 27, 2023

jaekwon Jul 27, 2023

jaekwon Jul 27, 2023

harry-hov Jul 31, 2023

jaekwon Jul 27, 2023 •

edited

Loading

harry-hov Jul 31, 2023

jaekwon Jul 27, 2023

jaekwon Jul 27, 2023

harry-hov Jul 31, 2023

jaekwon Jul 27, 2023

harry-hov Jul 31, 2023 •

edited

Loading

moul left a comment

	paths, err := gnoPackagesFromArgs(args)
	targets, err := targetsFromPatterns(args)

feat: fix gno test for _test.gno and _filetest.gno files #945

feat: fix gno test for _test.gno and _filetest.gno files #945

Conversation

harry-hov commented Jul 4, 2023 • edited Loading

Contributors Checklist

Maintainers Checklist

zivkovicmilos left a comment

Choose a reason for hiding this comment

tbruyelle left a comment

Choose a reason for hiding this comment

ajnavarro left a comment

Choose a reason for hiding this comment

harry-hov commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harry-hov Jul 30, 2023 • edited 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

harry-hov Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harry-hov Aug 10, 2023 • edited 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

jaekwon Jul 27, 2023 • edited 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

harry-hov Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

moul left a comment

Choose a reason for hiding this comment

feat: fix `gno test` for `_test.gno` and `_filetest.gno` files #945

feat: fix `gno test` for `_test.gno` and `_filetest.gno` files #945

harry-hov commented Jul 4, 2023 •

edited

Loading

harry-hov Jul 30, 2023 •

edited

Loading

harry-hov Aug 10, 2023 •

edited

Loading

harry-hov Aug 10, 2023 •

edited

Loading

jaekwon Jul 27, 2023 •

edited

Loading

harry-hov Jul 31, 2023 •

edited

Loading