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

LIU-420: Add graph for Slurm client tests #7

Merged
merged 1 commit into from
Nov 21, 2024
Merged

LIU-420: Add graph for Slurm client tests #7

merged 1 commit into from
Nov 21, 2024

Conversation

myxie
Copy link
Collaborator

@myxie myxie commented Nov 21, 2024

Overview

The SlurmClient class in DALiUGE requires a specific format of physical graph template file: it needs a tuple/list of the file name, and then the actual JSON data for the graph.

For testing purposes, I have created a small example file that let's us test the SlurmClient initialization with the 'expected' structure of the file.

Summary by Sourcery

New Features:

  • Introduce a new test graph file for SlurmClient initialization testing.

Copy link

sourcery-ai bot commented Nov 21, 2024

Reviewer's Guide by Sourcery

This PR adds a new test graph file specifically formatted for testing the SlurmClient class. The graph follows the required structure of having a tuple/list containing the file name and JSON graph data.

Class diagram for SlurmClient and Test Graph

classDiagram
    class SlurmClient {
        +String fileName
        +String jsonData
        +init(String fileName, String jsonData)
    }
    class TestGraph {
        +String fileName
        +String jsonData
    }
    SlurmClient --> TestGraph : uses
    note for SlurmClient "SlurmClient requires a tuple/list with fileName and jsonData for initialization"
Loading

File-Level Changes

Change Details Files
Added a new test graph file for SlurmClient testing
  • Created a new HelloWorld simple physical graph template
  • Implemented the required tuple/list format for SlurmClient compatibility
eagle_test_graphs/daliuge_tests/engine/graphs/SLURM_HelloWorld_simplePG.graph

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @myxie - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please include the contents of the SLURM_HelloWorld_simplePG.graph file in the PR description or ensure it's visible in the diff. This will help verify the file follows the required format (tuple/list of filename and JSON data).
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@myxie myxie merged commit 8d4077d into master Nov 21, 2024
2 checks passed
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.

1 participant