convert sync tests to go #1389

charlierudolph · 2020-05-08T05:51:24Z

Opening for visibility, not ready for review

# Conflicts: # Makefile

# Conflicts: # Makefile # test/git_environment.go # test/steps/configuration_steps.go # test/steps/merge_steps.go

kevgo · 2020-05-11T13:09:28Z

In case it helps, since there are so many sync tests, it might be more manageable to do one pull request per subdirectory.

kevgo

@charlierudolph I really don't know how you always do this and am in awe. You have converted at least as many specs as I have over the last months, but you did in a couple of days. Some suggestions below, curious what you think.

kevgo · 2020-05-11T20:22:44Z

test/commit_table_builder.go

@@ -61,6 +61,13 @@ func (builder *CommitTableBuilder) Add(commit Commit, location string) {
 	}
 }

+// AddMany registers the given commits from the given location into this table.
+func (builder *CommitTableBuilder) AddMany(commits []Commit, location string) {
+	for _, commit := range commits {


This type of loop creates a copy of the current commit in each loop iteration. Which is fine for small values like when iterating lists of int. Commits are, however, pretty large objects that consist of several strings. Here is a version that uses the commits in the list, i.e. avoids making a copy:

for c := range commits { builder.Add(commits[c], location) }

Probably not particularly relevant in this context, just wanted to share.

Makes sense. Hmm, would using an array of commit pointers be equivalent in efficiency? If so, I think I'd prefer we switch to that

I'd prefer to not switch to pointers for everything. They have other side effects like additional indirection on access (=slowness), and possibly more work for the garbage collector. Plus, on 64 bit systems pointers require an additional 8 bytes for every entry.

I think it's better to use the loop it was designed in Go: with the loop index as the primary element, and using the optional copy of the current item only when that makes sense.

kevgo · 2020-05-11T20:28:17Z

test/git_environment.go

-	err = result.DeveloperRepo.Fetch()
-	if err != nil {
-		return &result, fmt.Errorf("cannot fetch: %w", err)
+	for _, repo := range []GitRepository{result.DeveloperRepo, result.CoworkerRepo} {


Since coworker repos are extremely rarely used, what do you think about creating them only when needed? This could be done via an AddCoworker method of GitEnvironment, similar to AddUpstream.

Sounds good

kevgo · 2020-05-11T20:29:43Z

test/git_environment.go

+func (env GitEnvironment) CreateTags(table *messages.PickleStepArgument_PickleTable) error {
+	columnNames := helpers.TableFields(table)
+	if columnNames[0] != "NAME" && columnNames[1] != "LOCATION" {
+		return fmt.Errorf("tag table must have columns NAME and LOCATION")


Nice that you do this check and provide a helpful error message!

kevgo · 2020-05-11T20:37:51Z

test/git_repository.go

@@ -279,6 +298,15 @@ func (repo *GitRepository) CurrentBranch() (result string, err error) {
 	return strings.TrimSpace(outcome.OutputSanitized()), nil
 }

+// CurrentFileContent provides the current content of a file.
+func (repo *GitRepository) CurrentFileContent(filename string) (result string, err error) {


Thoughts about naming this FileContent? We already have FileContentInCommit(she, filename), FilesInCommit(sha), and FilesInBranch. I think a FileContent method fits naturally in here, and the name makes clear that this is the content of the given file in the given repo.

Okay that works

kevgo · 2020-05-11T20:42:32Z

test/git_repository.go

+	if err != nil {
+		return result, fmt.Errorf("cannot determine files in branch %q in repo %q: %w", branch, repo.Dir, err)
+	}
+	for _, line := range strings.Split(strings.TrimSpace(outcome.OutputSanitized()), "\n") {


FYI Result.OutputSanitized already runs strings.TrimSpace on the output. Since this is wrong in a lot of my code as well (and you probably followed it as a reference here), I am cleaning this up in #1403, so nothing to do here except to accept my apologies.

Thanks for the cleanup!

kevgo · 2020-05-11T21:23:05Z

test/steps/tag_steps.go

+
+// TagSteps defines Gherkin step implementations around merges.
+func TagSteps(suite *godog.Suite, fs *FeatureState) {
+	suite.Step(`^I have the following tags$`, func(table *messages.PickleStepArgument_PickleTable) error {


People don't have tags 🙂 Thoughts about renaming this to my repo has the following tags? Also okay to do efficiently as a search-and-replace once the conversion is done.

kevgo · 2020-05-11T21:24:12Z

test/tag_table_bulider.go

@@ -0,0 +1,61 @@
+package test


Typo in filename tag_table_bulider.go

kevgo · 2020-05-11T21:26:13Z

test/tag_table_bulider.go

+	result := TagTableBuilder{
+		tagToLocations: make(map[string]helpers.OrderedStringSet),
+	}
+	return result


You could inline the result variable.

kevgo · 2020-05-11T21:33:05Z

test/tag_table_bulider.go

+
+// AddMany registers the given tags from the given location into this table.
+func (builder *TagTableBuilder) AddMany(tags []string, location string) {
+	for _, tag := range tags {


Consider more efficient loop here.

kevgo · 2020-05-11T21:35:53Z

test/tag_table_bulider.go

+// Table provides the data accumulated by this TagTableBuilder as a DataTable.
+func (builder *TagTableBuilder) Table() (result DataTable) {
+	result.AddRow("NAME", "LOCATION")
+	tags := make([]string, len(builder.tagToLocations))


Nice preallocation at the correct length!

Linter made me do it haha

charlierudolph added 2 commits May 7, 2020 22:49

sync wip 1

2f7997a

Merge branch 'master' into cr-sync

5d86113

# Conflicts: # Makefile

charlierudolph requested a review from kevgo May 8, 2020 05:51

charlierudolph added 11 commits May 8, 2020 23:42

wip all branches

84b34b2

all branches passing

d431348

passing, 11 undefined

b62c289

local perennial branches

79b41b9

only cowork steps left

3f82adc

Merge branch 'master' into cr-sync

e7e2c56

all passing

0ed478b

kill features must specify local

19db28e

revert

b5f69db

Merge branch 'master' into cr-sync

152e7e8

fix lint

2b21823

charlierudolph changed the title ~~WIP: convert sync tests to go~~ convert sync tests to go May 10, 2020

charlierudolph added 8 commits May 10, 2020 16:37

fix

ed2ab54

fix lint

74a1a8b

fix missing def

f10c6b6

Merge branch 'master' into cr-sync

4e968aa

# Conflicts: # Makefile # test/git_environment.go # test/steps/configuration_steps.go # test/steps/merge_steps.go

fix build

443692c

fix build

459ab29

fix build

43010b8

fix feature

1cf7baf

charlierudolph added 3 commits May 11, 2020 09:06

add comments / cleanup

7d2ee4a

revert temp change

e8eed6d

fi

fa36892

charlierudolph merged commit c133086 into master May 11, 2020

charlierudolph deleted the cr-sync branch May 11, 2020 16:15

kevgo reviewed May 11, 2020

View reviewed changes

This was referenced May 12, 2020

rework coworker repo #1409

Merged

update tag steps #1410

Merged

various cleanup #1411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert sync tests to go #1389

convert sync tests to go #1389

charlierudolph commented May 8, 2020

kevgo commented May 11, 2020

kevgo left a comment

kevgo May 11, 2020

charlierudolph May 12, 2020

kevgo May 12, 2020

kevgo May 11, 2020

charlierudolph May 12, 2020

kevgo May 11, 2020

kevgo May 11, 2020

charlierudolph May 12, 2020

kevgo May 11, 2020 •

edited

charlierudolph May 12, 2020

kevgo May 11, 2020 •

edited

kevgo May 11, 2020

kevgo May 11, 2020

kevgo May 11, 2020

kevgo May 11, 2020

charlierudolph May 12, 2020

convert sync tests to go #1389

convert sync tests to go #1389

Conversation

charlierudolph commented May 8, 2020

kevgo commented May 11, 2020

kevgo left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevgo May 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevgo May 11, 2020 • edited

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

kevgo May 11, 2020 •

edited

kevgo May 11, 2020 •

edited