From 902fafb8e7216d0f5064667452b49dbc0d267e99 Mon Sep 17 00:00:00 2001 From: "Fuente, Pablo Andres" Date: Sun, 9 Jul 2017 01:34:50 -0300 Subject: [PATCH 1/4] Fixing tests for Windows Fixing the config/config_test, the discovery/file/file_test and the promql/promql_test tests for Windows. For most of the tests, the fix involved correct handling of path separators. In the case of the promql tests, the issue was related to the removal of the temporal directories used by the storage. The issue is that the RemoveAll() call returns an error when it tries to remove a directory which is not empty, which seems to be true due to some kind of process that is still running after closing the storage. To fix it I added some retries to the remove of the temporal directories. Adding tags file from Universal Ctags to .gitignore --- .gitignore | 1 + config/config_non_windows_test.go | 27 +++++++++++ config/config_test.go | 45 ++++++++++++++----- config/config_windows_test.go | 25 +++++++++++ config/testdata/conf.good.yml | 1 - config/testdata/rules_abs_path.good.yml | 4 ++ .../testdata/rules_abs_path_windows.good.yml | 4 ++ discovery/file/file_test.go | 5 ++- util/testutil/directory.go | 14 ++++-- 9 files changed, 109 insertions(+), 17 deletions(-) create mode 100644 config/config_non_windows_test.go create mode 100644 config/config_windows_test.go create mode 100644 config/testdata/rules_abs_path.good.yml create mode 100644 config/testdata/rules_abs_path_windows.good.yml diff --git a/.gitignore b/.gitignore index 2fe870e94..4d417605a 100644 --- a/.gitignore +++ b/.gitignore @@ -13,6 +13,7 @@ .*.swo *.iml .idea +tags /prometheus /promtool diff --git a/config/config_non_windows_test.go b/config/config_non_windows_test.go new file mode 100644 index 000000000..8e0be7243 --- /dev/null +++ b/config/config_non_windows_test.go @@ -0,0 +1,27 @@ +// Copyright 2015 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +build !windows + +package config + +const ruleFilesConfigFile = "testdata/rules_abs_path.good.yml" +var ruleFilesExpectedConf = &Config{ + GlobalConfig: DefaultGlobalConfig, + RuleFiles: []string{ + "testdata/first.rules", + "testdata/rules/second.rules", + "/absolute/third.rules", + }, + original: "", +} diff --git a/config/config_test.go b/config/config_test.go index 0282deb0e..8071d7258 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -17,6 +17,7 @@ import ( "encoding/json" "io/ioutil" "net/url" + "path/filepath" "reflect" "regexp" "strings" @@ -48,9 +49,8 @@ var expectedConf = &Config{ }, RuleFiles: []string{ - "testdata/first.rules", - "/absolute/second.rules", - "testdata/my/*.rules", + filepath.FromSlash("testdata/first.rules"), + filepath.FromSlash("testdata/my/*.rules"), }, RemoteWriteConfigs: []*RemoteWriteConfig{ @@ -85,7 +85,7 @@ var expectedConf = &Config{ Scheme: DefaultScrapeConfig.Scheme, HTTPClientConfig: HTTPClientConfig{ - BearerTokenFile: "testdata/valid_token_file", + BearerTokenFile: filepath.FromSlash("testdata/valid_token_file"), }, ServiceDiscoveryConfig: ServiceDiscoveryConfig{ @@ -252,9 +252,9 @@ var expectedConf = &Config{ TagSeparator: DefaultConsulSDConfig.TagSeparator, Scheme: "https", TLSConfig: TLSConfig{ - CertFile: "testdata/valid_cert_file", - KeyFile: "testdata/valid_key_file", - CAFile: "testdata/valid_ca_file", + CertFile: filepath.FromSlash("testdata/valid_cert_file"), + KeyFile: filepath.FromSlash("testdata/valid_key_file"), + CAFile: filepath.FromSlash("testdata/valid_ca_file"), InsecureSkipVerify: false, }, }, @@ -283,8 +283,8 @@ var expectedConf = &Config{ HTTPClientConfig: HTTPClientConfig{ TLSConfig: TLSConfig{ - CertFile: "testdata/valid_cert_file", - KeyFile: "testdata/valid_key_file", + CertFile: filepath.FromSlash("testdata/valid_cert_file"), + KeyFile: filepath.FromSlash("testdata/valid_key_file"), }, BearerToken: "mysecret", @@ -354,8 +354,8 @@ var expectedConf = &Config{ Timeout: model.Duration(30 * time.Second), RefreshInterval: model.Duration(30 * time.Second), TLSConfig: TLSConfig{ - CertFile: "testdata/valid_cert_file", - KeyFile: "testdata/valid_key_file", + CertFile: filepath.FromSlash("testdata/valid_cert_file"), + KeyFile: filepath.FromSlash("testdata/valid_key_file"), }, }, }, @@ -549,6 +549,29 @@ func TestLoadConfig(t *testing.T) { } +func TestLoadConfigRuleFilesAbsolutePath(t *testing.T) { + // Parse a valid file that sets a rule files with an absolute path + c, err := LoadFile(ruleFilesConfigFile) + if err != nil { + t.Errorf("Error parsing %s: %s", ruleFilesConfigFile, err) + } + + bgot, err := yaml.Marshal(c) + if err != nil { + t.Fatalf("%s", err) + } + + bexp, err := yaml.Marshal(ruleFilesExpectedConf) + if err != nil { + t.Fatalf("%s", err) + } + ruleFilesExpectedConf.original = c.original + + if !reflect.DeepEqual(c, ruleFilesExpectedConf) { + t.Fatalf("%s: unexpected config result: \n\n%s\n expected\n\n%s", ruleFilesConfigFile, bgot, bexp) + } +} + var expectedErrors = []struct { filename string errMsg string diff --git a/config/config_windows_test.go b/config/config_windows_test.go new file mode 100644 index 000000000..cb8dcebfb --- /dev/null +++ b/config/config_windows_test.go @@ -0,0 +1,25 @@ +// Copyright 2015 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package config + +const ruleFilesConfigFile = "testdata/rules_abs_path_windows.good.yml" +var ruleFilesExpectedConf = &Config{ + GlobalConfig: DefaultGlobalConfig, + RuleFiles: []string{ + "testdata\\first.rules", + "testdata\\rules\\second.rules", + "c:\\absolute\\third.rules", + }, + original: "", +} diff --git a/config/testdata/conf.good.yml b/config/testdata/conf.good.yml index 69c9cdc88..34da77d4e 100644 --- a/config/testdata/conf.good.yml +++ b/config/testdata/conf.good.yml @@ -10,7 +10,6 @@ global: rule_files: - "first.rules" -- "/absolute/second.rules" - "my/*.rules" remote_write: diff --git a/config/testdata/rules_abs_path.good.yml b/config/testdata/rules_abs_path.good.yml new file mode 100644 index 000000000..e9d3cf7f9 --- /dev/null +++ b/config/testdata/rules_abs_path.good.yml @@ -0,0 +1,4 @@ +rule_files: + - 'first.rules' + - 'rules/second.rules' + - '/absolute/third.rules' diff --git a/config/testdata/rules_abs_path_windows.good.yml b/config/testdata/rules_abs_path_windows.good.yml new file mode 100644 index 000000000..667411ff5 --- /dev/null +++ b/config/testdata/rules_abs_path_windows.good.yml @@ -0,0 +1,4 @@ +rule_files: + - 'first.rules' + - 'rules\second.rules' + - 'c:\absolute\third.rules' diff --git a/discovery/file/file_test.go b/discovery/file/file_test.go index f977e3b22..65c53c23b 100644 --- a/discovery/file/file_test.go +++ b/discovery/file/file_test.go @@ -17,6 +17,7 @@ import ( "fmt" "io" "os" + "path/filepath" "testing" "time" @@ -89,12 +90,12 @@ retry: if _, ok := tg.Labels["foo"]; !ok { t.Fatalf("Label not parsed") } - if tg.String() != fmt.Sprintf("fixtures/_test%s:0", ext) { + if tg.String() != filepath.FromSlash(fmt.Sprintf("fixtures/_test%s:0", ext)) { t.Fatalf("Unexpected target group %s", tg) } tg = tgs[1] - if tg.String() != fmt.Sprintf("fixtures/_test%s:1", ext) { + if tg.String() != filepath.FromSlash(fmt.Sprintf("fixtures/_test%s:1", ext)) { t.Fatalf("Unexpected target groups %s", tg) } break retry diff --git a/util/testutil/directory.go b/util/testutil/directory.go index 1672c5b01..d3c9c926f 100644 --- a/util/testutil/directory.go +++ b/util/testutil/directory.go @@ -26,6 +26,9 @@ const ( // NilCloser is a no-op Closer. NilCloser = nilCloser(true) + + // The number of times that a TemporaryDirectory will retry its removal + temporaryDirectoryRemoveRetries = 2 ) type ( @@ -84,15 +87,20 @@ func NewCallbackCloser(fn func()) Closer { } func (t temporaryDirectory) Close() { + retries := temporaryDirectoryRemoveRetries err := os.RemoveAll(t.path) - if err != nil { + for err != nil && retries > 0 { switch { case os.IsNotExist(err): - return + err = nil default: - t.tester.Fatal(err) + retries-- + err = os.RemoveAll(t.path) } } + if err != nil { + t.tester.Fatal(err) + } } func (t temporaryDirectory) Path() string { From 193dc472309c0c7a50222db46590d7288c27380e Mon Sep 17 00:00:00 2001 From: "Fuente, Pablo Andres" Date: Sun, 9 Jul 2017 02:43:33 -0300 Subject: [PATCH 2/4] Fixing code style to adhere gofmt --- config/config_non_windows_test.go | 3 ++- config/config_windows_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/config/config_non_windows_test.go b/config/config_non_windows_test.go index 8e0be7243..932643b88 100644 --- a/config/config_non_windows_test.go +++ b/config/config_non_windows_test.go @@ -1,4 +1,4 @@ -// Copyright 2015 The Prometheus Authors +// Copyright 2017 The Prometheus Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -16,6 +16,7 @@ package config const ruleFilesConfigFile = "testdata/rules_abs_path.good.yml" + var ruleFilesExpectedConf = &Config{ GlobalConfig: DefaultGlobalConfig, RuleFiles: []string{ diff --git a/config/config_windows_test.go b/config/config_windows_test.go index cb8dcebfb..ac97d3105 100644 --- a/config/config_windows_test.go +++ b/config/config_windows_test.go @@ -1,4 +1,4 @@ -// Copyright 2015 The Prometheus Authors +// Copyright 2017 The Prometheus Authors // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -14,6 +14,7 @@ package config const ruleFilesConfigFile = "testdata/rules_abs_path_windows.good.yml" + var ruleFilesExpectedConf = &Config{ GlobalConfig: DefaultGlobalConfig, RuleFiles: []string{ From fe73de94525d70ea3a044f82123381e775910e54 Mon Sep 17 00:00:00 2001 From: "Fuente, Pablo Andres" Date: Mon, 10 Jul 2017 00:02:08 -0300 Subject: [PATCH 3/4] Renaming config test file to fix build tags Renaming the name of a file of the config tests, in order to properly use the Go build tags feature. --- config/{config_non_windows_test.go => config_notwin_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename config/{config_non_windows_test.go => config_notwin_test.go} (100%) diff --git a/config/config_non_windows_test.go b/config/config_notwin_test.go similarity index 100% rename from config/config_non_windows_test.go rename to config/config_notwin_test.go From 9eb8c6e1d2f0c52e4f9b3a6a5362970a043c08d3 Mon Sep 17 00:00:00 2001 From: "Fuente, Pablo Andres" Date: Mon, 10 Jul 2017 11:08:16 -0300 Subject: [PATCH 4/4] Renaming the config_notwin test to config_default --- config/{config_notwin_test.go => config_default_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename config/{config_notwin_test.go => config_default_test.go} (100%) diff --git a/config/config_notwin_test.go b/config/config_default_test.go similarity index 100% rename from config/config_notwin_test.go rename to config/config_default_test.go