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 async #82

Merged
merged 9 commits into from
Sep 18, 2023
Merged

Fix async #82

merged 9 commits into from
Sep 18, 2023

Conversation

antonio-gg-dev
Copy link
Member

@antonio-gg-dev antonio-gg-dev commented Sep 14, 2023

πŸ“š Description

Replace this text with a short description of your feature/bugfix.

πŸ”– Changes

  • List individual changes in more detail

βœ… To-do list

  • Make sure that all the pipeline passes
  • Make sure to update the CHANGELOG.md to reflect the new feature or fix

@antonio-gg-dev antonio-gg-dev changed the title Extract state usage to functions Fix async Sep 14, 2023
@antonio-gg-dev
Copy link
Member Author

@Chemaclass and I have undergone a refactor to extract the application's state logic into a separate file (commit c50b5f7). After this, we tested various systems, providing a brief report on each one:

  • tmpfile: We encountered race conditions. Two subprocesses read and wrote the file at the same time, and the last one to write was the winner.
  • mkfifo: This one initially seemed to work perfectly during a test, but after that, the process began to hang, throwing errors that the files already existed. It seemed the best choice but stopped working unexpectedly.
  • shm: The same issue as with tmpfile arose, but the execution was much faster since data storage was in RAM.
  • shm + flock: This is the implementation currently in the latest commit (2adae5e), though it's only implemented in the passed_tests. When trying to implement it for the entire state, the tests began to fail. The reason needs a deeper investigation.

Having said that, we've left the work half done for today in case anyone wants to investigate further. In the worst-case scenario, if we decided to abandon the asynchronous execution feature, we could at least retain the extract state refactor.

I would like to add that even if we implement a system compatible with asynchronous operation, I believe we should insert an 'if' in all getters and setters to use the implementation from commit c50b5f7 (which simply uses variables) if the user runs the tests synchronously, as it seems to be the fastest implementation of all.

@antonio-gg-dev
Copy link
Member Author

@Chemaclass @khru Can you help me with the test test_when_running_bashunit_without_error_then_exit_code_should_be_success? It suddenly broke, and I don't know why. That's the only thing left for me to remove the draft.

@antonio-gg-dev antonio-gg-dev linked an issue Sep 17, 2023 that may be closed by this pull request
@khru
Copy link
Member

khru commented Sep 18, 2023

@Chemaclass @khru Can you help me with the test test_when_running_bashunit_without_error_then_exit_code_should_be_success? It suddenly broke, and I don't know why. That's the only thing left for me to remove the draft.

@Tito-Kati I think for some reason now it's returning this
Your branch:
image

Main
image

I've tested like this:

function test_when_running_bashunit_without_error_then_exit_code_should_be_success() {
  t="$(./bashunit)"
  echo $t
  assertSuccessfulCode "$(./bashunit)"
}

@antonio-gg-dev
Copy link
Member Author

It's true that it's a bit odd to run the command without specifying the files to test. How should we proceed? @khru @Chemaclass

@khru
Copy link
Member

khru commented Sep 18, 2023

Personally I don't have a preference, I like that if it doesn't give you a file it tells you executed 0 files, but I don't care if the final result doesn't appear because we are already saying that we have nothing to execute. But I do think should end with an exitcode 1.

Maybe see how jest and PHPUnit behave could help us

@Chemaclass
Copy link
Member

I would say that if you dont pass any arguments (path to run) it should do an exit 1, but I wouldnt bother a lot about this right now. We will luckily change this implementation later, when we could define in a config file the path, so you could run bashunit without especifying all the time the path, because it will read it from the config. So, I wont consider this a blocker for this PR.

@antonio-gg-dev
Copy link
Member Author

So... removing that test?

@antonio-gg-dev antonio-gg-dev marked this pull request as ready for review September 18, 2023 15:42
@antonio-gg-dev antonio-gg-dev added the bug Something isn't working label Sep 18, 2023
@Chemaclass Chemaclass merged commit 7abeda6 into main Sep 18, 2023
7 checks passed
@Chemaclass Chemaclass deleted the fix-async branch September 18, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Total assertions count not working when parallel run enabled
4 participants