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

Add process dump for failed integration tests #4304

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

pchila
Copy link
Member

@pchila pchila commented Feb 21, 2024

What does this PR do?

This PR adds a dump of the processes running on the system when an integration test fails and saves it in a JSON file available as artifact

Why is it important?

We are often encountering strange behavior in integration tests which can be attributed to extra processes running on the system and investigation benefits from having a picture of what was running on the system close to the time of failure.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added an entry in ./changelog/fragments using the changelog tool
  • [ ] I have added an integration test or an E2E test

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@mergify mergify bot assigned pchila Feb 21, 2024
Copy link
Contributor

mergify bot commented Feb 21, 2024

This pull request does not have a backport label. Could you fix it @pchila? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@pchila pchila force-pushed the add-processes-dump-for-failed-tests branch 2 times, most recently from 5b77394 to b153f35 Compare February 22, 2024 09:12
@pchila pchila added Testing Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team labels Feb 22, 2024
@pchila pchila requested a review from rdner February 22, 2024 09:18
@pchila pchila marked this pull request as ready for review February 22, 2024 09:18
@pchila pchila requested a review from a team as a code owner February 22, 2024 09:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@pchila pchila force-pushed the add-processes-dump-for-failed-tests branch from c26d8da to b153f35 Compare February 22, 2024 09:33
// Sub-test names are separated by "/" characters which are not valid filenames on Linux.
sanitizedTestName := strings.ReplaceAll(f.t.Name(), "/", "-")

filePath := filepath.Join(dir, "build", "diagnostics", fmt.Sprintf("TEST-%s-ProcessDump.json", sanitizedTestName))
Copy link
Member

Choose a reason for hiding this comment

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

Did you manage to see a build where this artifact was attached? Can you link it here?

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.

Great! Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

We probably want to put the test architecture in the artifact name. You'll be able to tell windows from linux by the contents, but not arm vs x86 linux.

Diagnostics have the same naming problem, but the variables file tells you the host arch when you open them.

Copy link
Member Author

Choose a reason for hiding this comment

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

added os and arch in 1f9348a

@pchila pchila force-pushed the add-processes-dump-for-failed-tests branch from b153f35 to b40fccc Compare February 22, 2024 12:00
@pchila pchila requested a review from rdner February 22, 2024 12:11
@pchila pchila force-pushed the add-processes-dump-for-failed-tests branch from b40fccc to 5104b03 Compare February 22, 2024 12:11
@pchila pchila force-pushed the add-processes-dump-for-failed-tests branch from e97544b to 1f9348a Compare February 22, 2024 16:03
Copy link

Quality Gate passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
No Duplication information No data about Duplication

See analysis details on SonarQube

@pchila pchila merged commit da59d1e into elastic:main Feb 22, 2024
9 checks passed
@pchila pchila deleted the add-processes-dump-for-failed-tests branch February 22, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants