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

#172 error messages should state version of cwltool being used #323

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oceenachi
Copy link

@oceenachi oceenachi commented Apr 13, 2021

Description

Following #172, when there is an error in parsing a given workflow it is expected that the version of cwl tool used in parsing that workflow is returned in the error body.
I also removed a portion of redundant code versionhere in loading.html

Motivation and Context

The change is required because it fixes the problem of cwltoolVersion not being shown when there is a parsing error as described in #172

#172

How Has This Been Tested?

Firstly I made sure I reproduced the error as shown on #172. After making changes, I started up my local environment and tried to parse the same URL as used in reproducing the error. Then I looked for changes in the error body. The difference is shown in the before vs after images below

Screenshots (if appropriate):

Previous image
image

After code change
image

Types of changes

  • [ X] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mr-c mr-c linked an issue Apr 15, 2021 that may be closed by this pull request
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

This is great, thank you! Can you write a test to confirm your fix?

@oceenachi
Copy link
Author

Okay will do so

@oceenachi
Copy link
Author

Hey @mr-c, I'm struggling with how to go about writing the test. First of all, I don't know if the test should be on WorkflowService which is the class I made the fix on or the Workflow controller which sends the version to loading.html.

I tried creating the test on workflow service to verify that the version is set when the retryCwltool() is called, but retryCwltool returns void so I can't test that the value of cwltoolVersion changes.

Can you help me with ideas of how I can go about this?

Thanks

@mr-c mr-c requested a review from tetron July 23, 2021 09:29
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Great improvement!

I went as far as checking out the branch locally, but didn't manage to reproduce the warning message. If there's a step-by-step guide on how to reproduce it I can try it. I used some bogus files to enter into some parts of the code while debugging.

  • Left a note on the existing QueueWorkflow#getCwltoolVersion method. Unless there's a good reason for not using it in the retryCwltool, I think we should use it to avoid confusing other developers;
  • We are not handling a case where cwltool was not found. When that happens (I use a Python virtual env, so that always happens first, then I remember to source venv/bin/activate 😬 ) QueueWorkflow#getCwltoolVersion returns blank, and QueueWorkflow#getTempRepresentation#getCwltoolVersion returns a string saying there was an error accessing cwltool. This can be handled in a follow-up issue I think, where we show a clear message in the UI that something is wrong with cwltool;
  • There is an empty unit test in WorkflowServiceTest called retryCwltoolGeneration. I think we can conveniently use that unit test @chapmanb . Here's an example of how that should look like more or less:
    /**
     * Retry the running of cwltool
     */
    @Test
    public void retryCwltoolGeneration() {
        final CWLToolRunner cwlToolRunner = Mockito.mock(CWLToolRunner.class);
        // Create service under test
        final WorkflowService testWorkflowService = new WorkflowService(
                Mockito.mock(GitService.class),
                Mockito.mock(CWLService.class),
                Mockito.mock(WorkflowRepository.class),
                Mockito.mock(QueuedWorkflowRepository.class),
                Mockito.mock(ROBundleFactory.class),
                Mockito.mock(GraphVizService.class),
                cwlToolRunner,
                Mockito.mock(GitSemaphore.class),
                -1);
        final QueuedWorkflow queuedWorkflow = Mockito.mock(QueuedWorkflow.class);
        Workflow workflow = new Workflow();
        workflow.setCwltoolVersion("1.2.3.4.5");
        Mockito.when(queuedWorkflow.getTempRepresentation()).thenReturn(workflow);
        testWorkflowService.retryCwltool(queuedWorkflow);
        // TODO: finish writing the test
    }

Thanks!

@kinow
Copy link
Member

kinow commented Dec 20, 2021

Ping @oceenachi

We are cutting a release soon, but with another one to come in the next weeks/months. Keen to include this one if you have time to reply on the latest feedback 👍

Thanks!

@oceenachi
Copy link
Author

Ping @oceenachi

We are cutting a release soon, but with another one to come in the next weeks/months. Keen to include this one if you have time to reply on the latest feedback 👍

Thanks!

Hi @kinow,

Don't know if I am replying late or I can still make the fixes. Please do let me know thanks

@kinow
Copy link
Member

kinow commented Dec 24, 2021

Hi @oceenachi !

Don't know if I am replying late or I can still make the fixes. Please do let me know thanks

We just released CWL Viewer, however, we already have more changes lined up for the next release! So anytime you can make the fixes for this PR, we should be able to review in a few days (holidays season is on us after all 🎄 ) and include it in one of the next releases.

Thanks!
-Bruno

@kinow kinow force-pushed the ft-trial branch 2 times, most recently from 548c82c to 733261f Compare October 1, 2024 18:37
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Rebased squashing the changes. Tested locally but things didn't go well; got a PostgreSQL error, which I think might look similar to the Spring3 migration that I had... or might be something else. Weird. Will investigate more later 👍

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.

error messages should state version of cwltool being used
3 participants