> > - wrote an intentionally failing TAP test to see if "make check" executes >> it. it does not. need help. >> > > A failing test expects code 3, not 0 as written in "t/001_if.pl". With > > is($retcode,'3','Invalid \if respects ON_ERROR_STOP'); > > instead, the tests succeeds because psql returned 3. >
Right. What I meant was, no matter how I changed that test, I could not get it to fail, which made me think it was not executing at all. What do I need to do to get TAP tests running? I must be missing something. > More check should be done about stdout to check that it failed for the > expected reasons though. And maybe more tests could be added to trigger all > possible state transition errors (eg else after else, elif after else, > endif without if, if without endif, whatever...). Agreed. But I couldn't build further on the test without being sure it was being run. > > Other comments and suggestions: > > Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"? > I think that's a merge error from rebasing. Will fix. > > There is a spurious empty line added at the very end of "mainloop.h": > > + > #endif /* MAINLOOP_H */ > Not in my diff, but that's been coming and going in your diff reviews. > > > I would suggest to add a short one line comment before each test to > explain what is being tested, like "-- test \elif execution", "-- test > \else execution"... > Where are you suggesting this? > > Debatable suggestion about "psql_branch_empty": the function name somehow > suggests an "empty branch", where it is really testing that the stack is > empty. Maybe the function could be removed and "psql_branch_get_state(...) > == IF_STATE_NONE" used instead. Not sure. > The name isn't great. Maybe psql_branch_stack_empty()? "psql_branch_end_state": it is a pop, it could be named "psql_branch_pop" > which would be symmetrical to "psql_branch_push"? Or maybe push should be > named "begin_state" or "new_state"... > Yeah, we either need to go fully with telling the programmer that it's a stack (push/pop/empty) or (begin_branch/end_branch/not_branching). I'm inclined to go full-stack, as it were. > > C style details: I would avoid non mandatory parentheses, eg: > > + return ((strcmp(cmd, "if") == 0 || \ > + strcmp(cmd, "elif") == 0 || \ > + strcmp(cmd, "else") == 0 || \ > + strcmp(cmd, "endif") == 0)); > > I would remove the external double parentheses (( ... )). Also I'm not > sure why there are end-of-line backslashes on the first instance, maybe a > macro turned into a function? > I copied that from somewhere, and I don't remember where. I think the test was originally nested much further before being moved to its own function. Can fix. > > + return ((s == IFSTATE_NONE) || > + (s == IFSTATE_TRUE) || > + (s == IFSTATE_ELSE_TRUE)); > > I would remove all parenthenses. > +1 > > + return (state->branch_stack == NULL); > > Idem. +1
