Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ability to filter with case insensitive tags #2039

Merged
merged 7 commits into from
Jul 9, 2021
Merged

Ability to filter with case insensitive tags #2039

merged 7 commits into from
Jul 9, 2021

Conversation

haroon-sheikh
Copy link
Contributor

@haroon-sheikh haroon-sheikh commented Jul 7, 2021 •
edited
Loading

Signed-off-by: Haroon Sheikh haroon.sheikh@outlook.com

@haroon-sheikh
Ability to filter with case in-sensitive tags
bb88c77 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@haroon-sheikh haroon-sheikh changed the title Ability to filter with case in-sensitive tags Ability to filter with case insensitive tags Jul 7, 2021
@haroon-sheikh
Ability to filter with case in-sensitive tags
25c4f79 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@haroon-sheikh
Ability to filter with case in-sensitive tags
7f6dcd7 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
zabil
zabil reviewed Jul 8, 2021
View reviewed changes
filter/specItemFilter.go
@@ -70,7 +70,7 @@ func sanitize(tag string) string {
if _, err := strconv.ParseBool(tag); err == nil {
return fmt.Sprintf("{%s}", tag)
}
return tag
return strings.ToLower(tag)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @haroon-sheikh can we add a new property to make this a configuration? By default it's case sensitive with the option to turn it off. Worth observing if anyone uses case sensitive tag and making case insensitivity the default option in a future release.

Copy link
Contributor Author

@haroon-sheikh haroon-sheikh Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is, if we turn it off by default then its very unlikely to be used by others and we'll never find out if they're using it in a case sensitive way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added a flag around it. Let me know what you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. We can turn it on by default. Will take a look at the PR soon.

@haroon-sheikh
Adds case insensitivity behind a flag
65a023f 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@haroon-sheikh
Adds case insensitivity behind a flag
2eae431 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@haroon-sheikh
Adds case insensitivity behind a flag
e007613 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
zabil
zabil requested changes Jul 8, 2021
View reviewed changes
env/env.go Outdated
@@ -38,6 +38,7 @@ const (
saveExecutionResult = "save_execution_result"
// CsvDelimiter holds delimiter used to parse csv files
CsvDelimiter = "csv_delimiter"
allowCaseInSensitiveTags = "allow_case_insensitive_tags"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readabioity better to call this

Suggested change
allowCaseInSensitiveTags = "allow_case_insensitive_tags"
allowCaseSensitiveTags = "allow_case_sensitive_tags"

And set it as false by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

env/env.go Outdated
@@ -115,6 +116,7 @@ func loadDefaultEnvVars() {
defaultScreenshotDir := filepath.Join(config.ProjectRoot, common.DotGauge, "screenshots")
addEnvVar(GaugeScreenshotsDir, defaultScreenshotDir)
addEnvVar(gaugeSpecFileExtensions, ".spec, .md")
addEnvVar(allowCaseInSensitiveTags, "false")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
addEnvVar(allowCaseInSensitiveTags, "false")
addEnvVar(allowCaseISensitiveTags, "false")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

env/env.go Outdated
@@ -309,3 +311,8 @@ var GaugeSpecFileExtensions = func() []string {
}
return allowedExts
}

// AllowCaseInSensitiveTags determines if the casing is ignored in tags filtering
var AllowCaseInSensitiveTags = func() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var AllowCaseInSensitiveTags = func() bool {
var AllowCaseISensitiveTags = func() bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

env/env_test.go Outdated
@@ -39,6 +39,7 @@ func (s *MySuite) TestLoadDefaultEnv(c *C) {
defaultScreenshotDir := filepath.Join(config.ProjectRoot, common.DotGauge, "screenshots")
c.Assert(os.Getenv("gauge_screenshots_dir"), Equals, defaultScreenshotDir)
c.Assert(os.Getenv("gauge_spec_file_extensions"), Equals, ".spec, .md")
c.Assert(os.Getenv("allow_case_insensitive_tags"), Equals, "false")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c.Assert(os.Getenv("allow_case_insensitive_tags"), Equals, "false")
c.Assert(os.Getenv("allow_case_sensitive_tags"), Equals, "false")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

filter/specItemFilter.go Outdated
@@ -70,6 +71,9 @@ func sanitize(tag string) string {
if _, err := strconv.ParseBool(tag); err == nil {
return fmt.Sprintf("{%s}", tag)
}
if env.AllowCaseInSensitiveTags() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to use AllowCaseInSensitiveTags

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

filter/specItemFilter_test.go Outdated
@@ -19,6 +20,15 @@ type MySuite struct{}

var _ = Suite(&MySuite{})

func before() {
os.Clearenv()
os.Setenv("allow_case_insensitive_tags", "true")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
os.Setenv("allow_case_insensitive_tags", "true")
os.Setenv("allow_case_sensitive_tags", "true")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

version/version.go Outdated
@@ -14,7 +14,7 @@ import (
)

// CurrentGaugeVersion represents the current version of Gauge
var CurrentGaugeVersion = &Version{1, 3, 1}
var CurrentGaugeVersion = &Version{1, 3, 2}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another PR which will bump the release so you can remove this. Will merge this before #2041 @sriv

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@haroon-sheikh
Adds case insensitivity behind a flag
cd4ac33 
Signed-off-by: Haroon Sheikh <haroon.sheikh@sky.uk>
@zabil zabil merged commit 763a652 into getgauge:master Jul 9, 2021
@chadlwilson
Copy link
Contributor

chadlwilson commented Jan 2, 2022 •
edited
Loading

Thanks for fixing this! This seems to have highlighted that some specs for GoCD were not actually running as intended due to the previous case sensitivity :'(

chadlwilson added a commit to chadlwilson/ruby-functional-tests that referenced this pull request Jan 2, 2022
@chadlwilson
Correct expectations for ConfigRepoSPA spec
8c397f1 
Unfortunately, these specs were not running before due to case sensitivity in tags in Gauge. This case sensitivity was removed in getgauge/gauge#2039 (Gauge 1.3.2+) which means these specs are now running in earlier releases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zabil zabil zabil approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haroon-sheikh @chadlwilson @zabil