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

fix: GH workflow test #635

Merged
merged 16 commits into from
Jul 7, 2023
Merged

fix: GH workflow test #635

merged 16 commits into from
Jul 7, 2023

Conversation

mihai-sysbio
Copy link
Member

@mihai-sysbio mihai-sysbio commented Jun 13, 2023

Main improvements in this PR:

This PR is continuing #629 aimed to fix the GH workflow test of the import-export cycle, thus resolving #628.

I hereby confirm that I have:

  • Tested my code on my own computer for running the model
  • Selected develop as a target branch
  • Any removed reactions and metabolites have been moved to the corresponding deprecated identifier lists

mihai-sysbio and others added 2 commits June 13, 2023 14:20
- This commit includes fixing typos, and reverting back to original way of extracting subsystem values
@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 13, 2023

the reported error in yaml import/export cycle is fixed by correcting typos and reverting to original subsystem value extraction (e118e1d); while the issue with GH action remains, which might be resolved later?

Copy link
Member

@haowang-bioinfo haowang-bioinfo left a comment

Choose a reason for hiding this comment

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

I tested this PR by locally running testYamlConversion and tandemly import/export the yaml file, everything is fine.

would be good to have checks for unknown keys (caused by typos) in yaml, but have no idea how to reactivate the "idling" action

@mihai-sysbio
Copy link
Member Author

I'm still working on getting the workflow to fail in a visible way.

@mihai-sysbio mihai-sysbio force-pushed the fix/importExportCycle branch 2 times, most recently from e9426a9 to b247c0b Compare June 15, 2023 04:21
Copy link
Collaborator

@feiranl feiranl left a comment

Choose a reason for hiding this comment

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

LGTM!

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 16, 2023

could this be merged, how's going with the workflow test? @mihai-sysbio

@mihai-sysbio mihai-sysbio marked this pull request as draft June 16, 2023 08:52
@mihai-sysbio
Copy link
Member Author

could this be merged, how's going with the workflow test? @mihai-sysbio

The workflow still appears as passing in the latest workflow run even though it clearly prints out an error. I am having difficulties relaying the exit(1) from Matlab to bash. I've tried a few different things but none seemed to work. In the meantime, this PR is converted as to draft, since merging it would not resolve the intended issue.

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 16, 2023

In the meantime, this PR is converted as to draft, since merging it would not resolve the intended issue.

yes, let's do so

the aim of this PR can be reduced to only will continue in #629 resolving import/export cycle, which is a bug of develop branch that is now jammed

The workflow still appears as passing in the latest workflow run even though it clearly prints out an error. I am having difficulties relaying the exit(1) from Matlab to bash. I've tried a few different things but none seemed to work.

this workflow problem can be temporarily substituted with checking these Matlab-based tasks locally before merging

@haowang-bioinfo haowang-bioinfo marked this pull request as ready for review June 16, 2023 13:30
@haowang-bioinfo haowang-bioinfo changed the title fix: import export cycle and GH workflow test fix: import export cycle bug Jun 16, 2023
@haowang-bioinfo haowang-bioinfo marked this pull request as draft June 16, 2023 13:47
@haowang-bioinfo haowang-bioinfo changed the title fix: import export cycle bug fix: GH workflow test Jun 16, 2023
@mihai-sysbio mihai-sysbio force-pushed the fix/importExportCycle branch 2 times, most recently from e822985 to 6ac87c4 Compare June 19, 2023 13:50
@mihai-sysbio mihai-sysbio force-pushed the fix/importExportCycle branch from 6ac87c4 to 29113a1 Compare June 19, 2023 14:20
@mihai-sysbio
Copy link
Member Author

mihai-sysbio commented Jun 19, 2023

The workflow has been updated to fail in an obvious way if problems are encountered in commit 29113a1. The following commit 927fb07 removed the purposefully-introduced error, which resulted in all workflows passing. Therefore, the core issue is now resolved.

@mihai-sysbio
Copy link
Member Author

Due to significant changes and slight scope creep, reviews will be re-requested.

@haowang-bioinfo
Copy link
Member

if I can re-introduce the bug happened in Yaml before and see what happens?

@mihai-sysbio
Copy link
Member Author

mihai-sysbio commented Jun 20, 2023

if I can re-introduce the bug happened in Yaml before and see what happens?

good idea, please play around with different test cases

@mihai-sysbio
Copy link
Member Author

Another idea, should the disp function calls be replaced with warn?

- Test if GH action really work
@haowang-bioinfo
Copy link
Member

the bug re-troduced by 97d030b suppose to lead to failure of both yaml-validation and yaml-conversion actions. But the latter still gives a pass, therefore this workflow bug persists.

@mihai-sysbio
Copy link
Member Author

@haowang-bioinfo the reintroduced but caused a failure higher up in the testing code, which was not caught by the same mechanism. The previous commit 6e5d66c address this.

@mihai-sysbio
Copy link
Member Author

If there is any other bug you'd want to test with, please go ahead @haowang-bioinfo

@mihai-sysbio
Copy link
Member Author

Oh how interesting, none of tests capture the bug introduced in 5f39c61. The actions are passing, because the tests are passing. I would be inclined to report this as separate bug - the workflows now seem to reflect the status of the tests, which was the original aim of this PR.

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jul 3, 2023

go ahead please

This type of bug (5f39c61) probably can be detected by Yaml lint checker by providing a finite set of keys, from which any unknown keys could thus be detected and reported.

@mihai-sysbio mihai-sysbio mentioned this pull request Jul 3, 2023
@mihai-sysbio
Copy link
Member Author

PR #671 is aimed at merging into this one, and will catch the bug introduced in 5f39c61.

@mihai-sysbio
Copy link
Member Author

@haowang-bioinfo the purposefully introduced bug has now been caught and may be removed.

code/test/testMetabolicTasks.m Outdated Show resolved Hide resolved
code/test/testMetabolicTasks.m Outdated Show resolved Hide resolved
code/test/testYamlConversion.m Outdated Show resolved Hide resolved
code/test/testYamlConversion.m Outdated Show resolved Hide resolved
@haowang-bioinfo
Copy link
Member

with #672, both yaml-conversion and yaml-validation actions can catch the typo in MAM1261m

Copy link
Member

@haowang-bioinfo haowang-bioinfo left a comment

Choose a reason for hiding this comment

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

looks very good - the previously idle yaml-conversion action in functioning, together with enhanced memote checks

@mihai-sysbio mihai-sysbio merged commit f4f99d7 into develop Jul 7, 2023
@mihai-sysbio mihai-sysbio deleted the fix/importExportCycle branch July 7, 2023 03:49
@haowang-bioinfo haowang-bioinfo mentioned this pull request Sep 25, 2023
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.

3 participants