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.

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

feat(filter): Add error handling for regex compilation #933

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

Tetrergeru
Copy link
Member

No description provided.

@Tetrergeru Tetrergeru requested a review from a team as a code owner October 10, 2023 10:30
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2023

Codecov Report

Merging #933 (d19a381) into master (015eb55) will decrease coverage by 0.09%.
The diff coverage is 50.00%.

❗ Current head d19a381 differs from pull request most recent head befb374. Consider uploading reports for the commit befb374 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #933      +/-   ##
==========================================
- Coverage   68.94%   68.86%   -0.09%     
==========================================
  Files         196      196              
  Lines       11013    11023      +10     
==========================================
- Hits         7593     7591       -2     
- Misses       2977     2985       +8     
- Partials      443      447       +4     
Files Coverage Δ
filter/series_by_tag.go 90.99% <64.70%> (-5.28%) ⬇️
filter/series_by_tag_pattern_index.go 79.31% <14.28%> (-20.69%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -129,15 +129,19 @@ func ParseSeriesByTag(input string) ([]TagSpec, error) {
type MatchingHandler func(string, map[string]string) bool

// CreateMatchingHandlerForPattern creates function for matching by tag list
func CreateMatchingHandlerForPattern(tagSpecs []TagSpec, compatibility *Compatibility) (string, MatchingHandler) {
func CreateMatchingHandlerForPattern(tagSpecs []TagSpec, compatibility *Compatibility) (string, MatchingHandler, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Может стоит добавить тесты на эту функцию? Я просто посмотрел, там на часть функций из файла series_by_tag.go есть тесты, а на другую часть нет, не обязательно прям в этом пр добавлять, но в целом они бы не помешали, я думаю

Copy link
Member Author

Choose a reason for hiding this comment

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

Надо подумать, нужны ли тесты на эти функции или хватит тестов на самую выскокоуровневую. Тестируем-то логический модуль, а не каждую функцию по-отдельности. Но в этом ПР прям не хочется ещё тестов писать

Copy link
Member

Choose a reason for hiding this comment

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

Да, в этом пр давайте оставим пока так, это пока просто мысли на будущее :)

@almostinf almostinf self-requested a review October 10, 2023 13:10
}

func createMatchingHandlerForOneTag(spec TagSpec, compatibility *Compatibility) MatchingHandler {
func createMatchingHandlerForOneTag(spec TagSpec, compatibility *Compatibility) (MatchingHandler, error) {
Copy link
Member

Choose a reason for hiding this comment

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

На этой функции также тестов нет

Copy link
Member Author

Choose a reason for hiding this comment

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

Аналогично комменту выше

@Tetrergeru
Copy link
Member Author

/buil

@Tetrergeru
Copy link
Member Author

/build

@Tetrergeru Tetrergeru merged commit d9b2ab4 into master Oct 11, 2023
6 checks passed
@Tetrergeru Tetrergeru deleted the feature/error-handling-in-filter branch October 11, 2023 09:07
@github-actions
Copy link

Build and push Docker images with tag: 2023-10-11.d9b2ab4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants