Skip to content

Conversation

@CodexRaunak
Copy link
Contributor

Description

Performed clean up for integration tests:

  • Added waitForMeshsync(), validateErrorOutcome(), handleLibraryCompletion(), runMeshsyncLibraryAsync() methods to reduce cyclomatic complexity by separating the logic to helper functions.
  • Removed nil options from the test case, as we don't need a separate test for this.

This PR fixes #486

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @CodexRaunak, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enhancing the maintainability and clarity of the integration test suite. It refactors existing test logic into dedicated helper functions to decrease cyclomatic complexity and improve code organization. Additionally, it streamlines test cases by removing redundancy, making the test suite more efficient and easier to understand.

Highlights

  • Refactored Integration Tests: Extracted common logic into new helper functions (waitForMeshsync, runMeshsyncLibraryAsync, handleLibraryCompletion, validateErrorOutcome) to reduce cyclomatic complexity and improve readability in integration tests.
  • Streamlined Test Cases: Consolidated the test case for nil options in libmeshsync.OptionsSetter by integrating it into an existing test, removing a redundant test case.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@CodexRaunak CodexRaunak changed the title Cleanup integration tests Reduce cyclomatic complexity and clean up in integration tests Nov 23, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively cleans up the integration tests by refactoring complex logic into smaller, more manageable helper functions. This significantly improves the readability and maintainability of the test code. The removal of the redundant test case for nil options is also a good simplification. I have a couple of minor suggestions to further enhance the code's conciseness and efficiency.

Comment on lines 160 to 162
go func(cmd0 *exec.Cmd, errCh0 chan<- error) {
errCh0 <- cmd0.Wait()
}(cmd, errCh)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This goroutine can be simplified. Since cmd and errCh are not loop variables, you can safely capture them from the enclosing scope instead of passing them as arguments. This makes the code more concise and idiomatic.

Suggested change
go func(cmd0 *exec.Cmd, errCh0 chan<- error) {
errCh0 <- cmd0.Wait()
}(cmd, errCh)
go func() {
errCh <- cmd.Wait()
}()

errCh chan<- error,
) {
go func() {
runOptions := make([]libmeshsync.OptionsSetter, 0, len(tc.meshsyncRunOptions))
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To avoid a potential slice reallocation on line 148, you can initialize runOptions with enough capacity to hold all the elements from the start. This is a small performance optimization.

Suggested change
runOptions := make([]libmeshsync.OptionsSetter, 0, len(tc.meshsyncRunOptions))
runOptions := make([]libmeshsync.OptionsSetter, 0, len(tc.meshsyncRunOptions)+1)

@CodexRaunak CodexRaunak force-pushed the cleanup-integration-tests branch from 2567c62 to 38462a1 Compare November 23, 2025 20:58
@CodexRaunak
Copy link
Contributor Author

@n2h9
Would like a review on the changes made, and open to any suggestions.
Thank you for your time

Copy link
Contributor

@n2h9 n2h9 left a comment

Choose a reason for hiding this comment

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

Hello @CodexRaunak 👋 !

Looks good, thank you 🙇‍♀️ .

  1. There is a formatting issue reported by the linter, could you please address it (you probably need to run go fmt ).

  2. With this refactoring are we already ready to remove nolint statement from line 64 ?

@CodexRaunak
Copy link
Contributor Author

  1. With this refactoring are we already ready to remove nolint statement from line 64 ?

Upon running this command, perhaps it's not reporting the errors. Below are the logs

golangci-lint run -v

INFO golangci-lint has version v1.64.8 built with go1.25.4 from (unknown, modified: ?, mod sum: "h1:y5TdeVidMtBGG32zgSC7ZXTFNHrsJkDnpO4ItB3Am+I=") on (unknown) 
INFO [config_reader] Config search paths: [./ /Users/raunak/Projects/meshsync /Users/raunak/Projects /Users/raunak /Users /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [goenv] Read go env for 5.137625ms: map[string]string{"GOCACHE":"/Users/raunak/Library/Caches/go-build", "GOROOT":"/opt/homebrew/Cellar/go/1.25.4/libexec"} 
INFO [lintersdb] Active 10 linters: [cyclop errcheck gci goconst gocritic gosimple govet ineffassign staticcheck unused] 
INFO [loader] Go packages loading at mode 8767 (deps|exports_file|files|name|compiled_files|imports|types_sizes) took 638.129625ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 4.050708ms 
INFO [linters_context/goanalysis] analyzers took 0s with no stages 
INFO [runner/skip_dirs] Skipped 68 issues from dir internal/config by pattern config 
INFO [runner] Issues before processing: 83, after processing: 0 
INFO [runner] Processors filtering stat (in/out): cgo: 83/83, skip_files: 83/83, identifier_marker: 15/15, exclusion_paths: 83/83, exclusion_rules: 15/6, skip_dirs: 83/15, filename_unadjuster: 83/83, invalid_issue: 83/83, path_relativity: 83/83, generated_file_filter: 15/15, nolint_filter: 6/0, path_absoluter: 83/83 
INFO [runner] processing took 1.433333ms with stages: nolint_filter: 1.121125ms, generated_file_filter: 142.708µs, exclusion_rules: 104.875µs, skip_dirs: 32.542µs, path_relativity: 21.291µs, invalid_issue: 3.001µs, cgo: 2µs, path_absoluter: 1.791µs, filename_unadjuster: 1.25µs, max_same_issues: 958ns, fixer: 375ns, identifier_marker: 375ns, sort_results: 292ns, skip_files: 167ns, source_code: 125ns, uniq_by_line: 125ns, diff: 83ns, max_from_linter: 83ns, path_shortener: 42ns, max_per_file_from_linter: 42ns, path_prettifier: 42ns, exclusion_paths: 41ns, severity-rules: 0s 
INFO [runner] linters took 587.2345ms with stages: goanalysis_metalinter: 585.755542ms 
INFO File cache stats: 0 entries of total size 0B 
INFO Memory: 14 samples, avg is 65.2MB, max is 101.4MB 
INFO Execution took 1.240692209s                   

seems the configuration is preventing them from being reported, I tried altering the .golangci.yml, still not able to run the linter.

Maybe I am missing something, any help is appreciated.

@n2h9
Copy link
Contributor

n2h9 commented Nov 24, 2025

  1. With this refactoring are we already ready to remove nolint statement from line 64 ?

Upon running this command, perhaps it's not reporting the errors. Below are the logs

golangci-lint run -v

INFO golangci-lint has version v1.64.8 built with go1.25.4 from (unknown, modified: ?, mod sum: "h1:y5TdeVidMtBGG32zgSC7ZXTFNHrsJkDnpO4ItB3Am+I=") on (unknown) 
INFO [config_reader] Config search paths: [./ /Users/raunak/Projects/meshsync /Users/raunak/Projects /Users/raunak /Users /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [goenv] Read go env for 5.137625ms: map[string]string{"GOCACHE":"/Users/raunak/Library/Caches/go-build", "GOROOT":"/opt/homebrew/Cellar/go/1.25.4/libexec"} 
INFO [lintersdb] Active 10 linters: [cyclop errcheck gci goconst gocritic gosimple govet ineffassign staticcheck unused] 
INFO [loader] Go packages loading at mode 8767 (deps|exports_file|files|name|compiled_files|imports|types_sizes) took 638.129625ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 4.050708ms 
INFO [linters_context/goanalysis] analyzers took 0s with no stages 
INFO [runner/skip_dirs] Skipped 68 issues from dir internal/config by pattern config 
INFO [runner] Issues before processing: 83, after processing: 0 
INFO [runner] Processors filtering stat (in/out): cgo: 83/83, skip_files: 83/83, identifier_marker: 15/15, exclusion_paths: 83/83, exclusion_rules: 15/6, skip_dirs: 83/15, filename_unadjuster: 83/83, invalid_issue: 83/83, path_relativity: 83/83, generated_file_filter: 15/15, nolint_filter: 6/0, path_absoluter: 83/83 
INFO [runner] processing took 1.433333ms with stages: nolint_filter: 1.121125ms, generated_file_filter: 142.708µs, exclusion_rules: 104.875µs, skip_dirs: 32.542µs, path_relativity: 21.291µs, invalid_issue: 3.001µs, cgo: 2µs, path_absoluter: 1.791µs, filename_unadjuster: 1.25µs, max_same_issues: 958ns, fixer: 375ns, identifier_marker: 375ns, sort_results: 292ns, skip_files: 167ns, source_code: 125ns, uniq_by_line: 125ns, diff: 83ns, max_from_linter: 83ns, path_shortener: 42ns, max_per_file_from_linter: 42ns, path_prettifier: 42ns, exclusion_paths: 41ns, severity-rules: 0s 
INFO [runner] linters took 587.2345ms with stages: goanalysis_metalinter: 585.755542ms 
INFO File cache stats: 0 entries of total size 0B 
INFO Memory: 14 samples, avg is 65.2MB, max is 101.4MB 
INFO Execution took 1.240692209s                   

seems the configuration is preventing them from being reported, I tried altering the .golangci.yml, still not able to run the linter.

Maybe I am missing something, any help is appreciated.

The configuration on the line 64 tells to not lint cyclomatic complexity for the function.
If you remove it and run linter on master you will receive the linter error.
If the refactoring in this pr decreased cyclomatic complexity enough, we should remove this nolint instruction.

@github-actions github-actions bot added the language/go Golang related label Nov 25, 2025
@CodexRaunak
Copy link
Contributor Author

CodexRaunak commented Nov 25, 2025

The configuration on the line 64 tells to not lint cyclomatic complexity for the function. If you remove it and run linter on master you will receive the linter error. If the refactoring in this pr decreased cyclomatic complexity enough, we should remove this nolint instruction.

Updated, I didn't received any linting errors, therefore I removed the no lint statement and also fixed the formatting issues.

@n2h9 n2h9 merged commit 8173672 into meshery:master Nov 25, 2025
6 checks passed
@welcome
Copy link

welcome bot commented Nov 25, 2025

Thanks for your contribution to Meshery! 🎉

Meshery Logo
        Join the community, if you haven't yet and please leave a ⭐ star on the project. 😄

@CodexRaunak CodexRaunak deleted the cleanup-integration-tests branch November 25, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

language/go Golang related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean Up for Integration Tests

2 participants