Description of problem ====================== I found an assert like: rlRun "foo $(bar)" 1 "it's sunny" In case I investigated, it happened that one day `bar` returned huge but valid output, and eventually the command passed to `eval` inside rlRun was syntactically invalid. Now the problem is that eval returns 1 on syntax error, therefore the whole assert was *falsely* reported as PASS. Well apparently the code is somehow brittle, but these things happen and it should still be possible to catch that earlier, explode, and force tester to fix their code. I haven't tried this, but I guess that a simple check inside rlRun like echo "$command" | bash -n || explode "all your eval are belong to undefined" could have prevented the problem.
This is change of default behaviour, which shouldn't be changed. I suggest add global configuration variable like "beakerlib_rlRun_strict" and based on its value change the behaviour. unset or 0 => no change 1 => LogError on syntax error 2 => rlFail on syntax error I added this bug to the tracking BZ#1208577 "ho to do it better" where we can consult the methods of these settings.
> add global configuration variable like "beakerlib_rlRun_strict" No, please don't. Baseline: This is a bug[1]. rlRun is broken. (I really regret I filed it as RFE.) Now, if you think this is too big of a change[2], that's another question. If so, IMO the right way to handle it is to defer it to a later release, and warn users that this behavior will change in the future[3]. But please, for heaven's sake, don't add API that changes meaning of other API. ;) [1] In case it's not clear, please follow me: Purpose of the `rlRun` is to run the specified code and compare exit status of *that code*, expecting it to fir a hard-coded criterion. If a string has syntax error in it, Bash *will not* run it. Therefore there *is no* exit status of that code. It's bug in rlRun that it takes the code *from eval* and misinterprets it as status from the code in the string. There is no valid scenario where somebody could rely on this behavior. If there are tests outside that are affected, they are currently wrong, harming us, and should have always been failing (not warning but loudly exploding! :)). Only safe and honest way is to disable them. [2] I'd hesitate between "minor" and "patch" in SemVer scheme, that is, y++ or z++ in x.y.z [3] Frankly I'd be inclined not to even defer it, if people start complaining that their tests' behavior changed, it means that their tests were broken in the first place.
You completely forget testing of bash itself, it's errors in parser, testing new features etc. Then it isn't bug it is behaviour when rlRun try to resolve some expression and compare result with expected value. What about "command not found" error, could be rlRun checking if PATH is set correctly? This is bash and in bash it is programmers responsibility keep code executable or you can use switch "-e" or similar technique which are provided by bash. We would like to provide some options for easier way to keep your code in well conditions but without breaking backward compatibility and changing API.
(In reply to Jakub Prokes from comment #3) > You completely forget testing of bash itself, it's errors in parser, testing > new features etc. No I did not. You mean that Bash testers use same /bin/bash to run runtest.sh and their test code? Giving up control of their test to the rlRun developers? > Then it isn't bug it is behaviour when rlRun try to resolve some expression > and compare result with expected value. What about "command not found" > error, could be rlRun checking if PATH is set correctly? From man page: "Run command with optional comment and make sure its exit code matches expectations." Both name and manpage suggest that the purpose is *running*. That's the part that happens *after* the parsing, so if rlRun cannot do it, it should **not** pretend it did. Especially if it's so trivial not to, as I have demonstrated. About "Command not found"--at least that one has code 127, so there's greater chance that it will not be "expected". OTOH, it's something that you cannot check, or at least not so easily. > This is bash and in bash it is programmers responsibility keep code > executable I agree that programmer should never put funny code in there (OTOH you never know), but how does that change anything? It's rlRun that is obviously lying here. > or you can use switch "-e" or similar technique which are > provided by bash. I don't understand what -e has to do with it. And what you mean by similar techniique. > We would like to provide some options for easier way to keep your code in > well conditions but without breaking backward compatibility and changing API. If somebody relies on syntax error, that's not well conditions, that's just broken code. I don't think supporting broken code helps anybody.
(In reply to Alois Mahdal from comment #4) Beakerlib is not trying to solve problems all around the world. > From man page: > > "Run command with optional comment and make sure its exit code > matches expectations." > > Both name and manpage suggest that the purpose is *running*. That's the > part that happens *after* the parsing, so if rlRun cannot do it, it should > **not** pretend it did. Especially if it's so trivial not to, as I have > demonstrated. May be the documentation could be improved here but the true purpose is to execute some code using bash and handle its exit code. User may have various reasons to execute various code. User should know what is doing and should count with it while checking the exit code. He should check validity of the code by himself or, if beakerlib provides it, to ask for it. It should not be a default.
> May be the documentation could be improved here Sigh ... yes maybe the code could do what it says it does and not something else. I'm quite surprised that this is in "maybe/could/low_prio/wishful_thinking" zone. > but the true purpose is to execute some code using bash and handle its exit code. And yet it does not. rlRun ";;" rlRun says the exit status is the exit status of the code inside the parentheses, but it's not true. (Frankly I'm getting quite frustrated about the fact that you--maintainers--can't accept this simple fact. I might have share in that that I filed this as RFE but I tried to make it clear in my comments.) ~ Also, adding global variables that change behavior (or even meaning) of a function is dangerous as it almost always stands somewhere else than the function call so it leads to unreadable/misleading code. (Heck, they can be even passed from the environment.) Small step for developer, huge step for users. Backwards. You have been warned.
(In reply to Alois Mahdal from comment #6) > rlRun ";;" There are some limitations in bash we cannot do anything about that. In this case it is eval's fault. > Also, adding global variables that change behavior (or even meaning) of a > function is dangerous as it almost always stands somewhere else than the > function call so it leads to unreadable/misleading code. (Heck, they can be > even passed from the environment.) Small step for developer, huge step for > users. Backwards. You have been warned. We do not want to and even cannot change the default behavior as there are many people out there using it and if it is somehow buggy, while no bugs were filed until now, they are ok with it and count with it. That's the reason why we would like to implement it as optional feature which could be switched on and off.
(In reply to Dalibor Pospíšil from comment #7) > (In reply to Alois Mahdal from comment #6) > > rlRun ";;" > There are some limitations in bash we cannot do anything about that. You can do "echo $code | bash -n" as I have said in comment 0. > In this case it is eval's fault. It's *NOT* eval's fault if your rlRun says something that is not true. > > Also, adding global variables that change behavior (or even meaning) of a > > function is dangerous as it almost always stands somewhere else than the > > function call so it leads to unreadable/misleading code. (Heck, they can be > > even passed from the environment.) Small step for developer, huge step for > > users. Backwards. You have been warned. > We do not want to and even cannot change the default behavior as there are > many people out there using it and if it is somehow buggy, while no bugs > were filed until now, they are ok with it and count with it. That's the > reason why we would like to implement it as optional feature which could be > switched on and off. Oh here we go again, the good old "nobody complains, so everything is OK" logic. Do you realize that "they are ok with it" may be based on information corrupted by this bug? They have been lied to by rlRun. They don't *know* something is wrong. By the way do you have a single valid use case where people rely on this behavior? It might be good idea to have use case before introducing such a beast as global behavior-switching variable.
This message is a reminder that Fedora 21 is nearing its end of life. Approximately 4 (four) weeks from now Fedora will stop maintaining and issuing updates for Fedora 21. It is Fedora's policy to close all bug reports from releases that are no longer maintained. At that time this bug will be closed as EOL if it remains open with a Fedora 'version' of '21'. Package Maintainer: If you wish for this bug to remain open because you plan to fix it in a currently maintained version, simply change the 'version' to a later Fedora version. Thank you for reporting this issue and we are sorry that we were not able to fix it before Fedora 21 is end of life. If you would still like to see this bug fixed and are able to reproduce it against a later version of Fedora, you are encouraged change the 'version' to a later Fedora version prior this bug is closed as described in the policy above. Although we aim to fix as many bugs as possible during every release's lifetime, sometimes those efforts are overtaken by events. Often a more recent Fedora release includes newer upstream software that fixes bugs or makes them obsolete.
Still the same in FC22