[Bank account]: Add canonical data

[meatball133]

The goal with this pr is to bring so a higher percentage of the practice exercises has tests and also to make it easier for new tracks to implement the exercise.

Some tracks have taken the decision to add a concurrency test, I was a bit unsure how to implement it in the canonical-data.json since it needs for loops for a lot of tracks. And some tracks doesn't it make sense to test for concurrency.

Any way feedback appreciated

resolves: #554

[petertseng]

"is it possible" seems to imply a question, but the sentence here ends with a period instead of a question mark. Was it meant to be a question?

[meatball133]

Are we talking about the sentence on line 7-8?

[meatball133]

Since line 4 has a pretty similar wording

[meatball133]

I think I improved it, can you check again?

[petertseng]

Fortunately, in all cases where this file has an "expected": { "error": "..." }, it's always the case that it was the last operation that would cause the error.

If there were ever a case in the future where an operation other than the last one should cause the error, then putting it here would be unclear because it is unspecific about which operation should cause the error. In that case, it would be necessary to improve clarity by putting this "expected": { "error": "..." } in the operation that should cause the error, instead of here.

I cannot predict at this time whether there will ever be such a case in the future, so I currently recommend that reviewers continue to consider this pull request in its current form ready to be Approved (in the GitHub sense of that word), rather than refusing to Approve it based on these grounds.

If we were to discover that there is likely a need for such cases in the future, I would recommend making the change now so that all these cases don't have to be reimplemented.

[petertseng]

It might be a good idea to make this convention clear though - the comments would say something like "if an error is expected, the last operation is the one that caused it; all operations other than the last one should succeed"

[meatball133]

I will see if other reviewers feels like there needs to be a structure change if not I will add that as a comment that it is the last one that should always be the action which will raise error

[SleeplessByte]

My 2 cents

[SleeplessByte]

This But doesn't work because the previous case also has a but. I suggest we change the entire thing to:

This exercise is a good candidate to practice concurrency, which
may not be available or possible in the implementing language track.
In this case, all tests can be executed on a single thread. If there
are tests that don't work using a single thread, these should be
marked with a concurrency scenario.

We probably also want to add concurrency to https://github.com/exercism/problem-specifications/blob/main/SCENARIOS.txt.

[SleeplessByte]

If we're defining operations to have an operand / function call name, and optional argument of value, and an optional return expectation of expected, I think we should move error also into an operation expectation.

This also solves the potential ordering issue @petertseng mentions, and makes it easier to add interesting concurrency tests down the line.

[meatball133]

Thanks for your feedback, do you think writing it like this makes sense?

          {
            "operation": "deposit",
            "value": -50,
            "error": "amount must be greater than 0"
          }

[SleeplessByte]

Perhaps still nest it in expected like before? @petertseng what do you think?

[petertseng]

Is there a reason to prefer one over the other? Since I'm not currently aware of one, it seems we are empowered to make either choice. If a reason presents itself, then we would go with that one. For example, someone might say "all other error are currently under expected, and it is important for this to remain true because (they give a reason here)". You will notice that since I didn't have something to insert in the blank that I didn't put forth that reason.

[SleeplessByte]

Since no one has written any test generator for this format yet, I agree, there is no reason from that point of view right now to pick one or the other.

@meatball133 I'll leave it up to you unless someone else comes up with a good reason.

[SleeplessByte]

I would always add a comment for a future reader.

[petertseng]

unless we're testing the behaviour of the system following an error

This type of test could be quite valuable if one of the desired teaching points of this exercise is to ensure that the system remains in a usable state after an erroneous operation. Ideally that decision wouldn't need to be made now, but I do regret to say that since it might have an effect on how some of these tests might need to be written, it seems it's necessary to at least plan for it. If there is no intent at all to have this as a teaching point, then all is well and we can end this particular thread of discussion here, and reading the rest of this comment is unnecessary.

But if error handling is one of the desired teaching points of this exercise, then just as an example, consider a test that opens an account, deposits a negative amount (which is not allowed), and then checks that the balance is still zero following that erroneous operation:

1. Operations other than the last one could have an expected, with the outer expected still referring to the very last operation:

    {
      "uuid": "785f57e4-7c32-402e-a118-e3bfd4facf75",
      "description": "Bad deposit doesn't corrupt balance",
      "property": "bankAccount",
      "input": {
        "operations": [
          {
            "operation": "open"
          },
          {
            "operation": "deposit",
            "value": -50,
            "expected": {"error": "amount must be greater than 0"}
          },
          {
            "operation": "amount"
          }
        ]
      },
      "expected": 0
    }

2. Or, each operation could have its own expected and the outer one remains empty.

    {
      "uuid": "785f57e4-7c32-402e-a118-e3bfd4facf75",
      "description": "Bad deposit doesn't corrupt balance",
      "property": "bankAccount",
      "input": {
        "operations": [
          {
            "operation": "open"
          },
          {
            "operation": "deposit",
            "value": -50,
            "expected": {"error": "amount must be greater than 0"}
          },
          {
            "operation": "amount"
            "expected": 0
          }
        ]
      },
      "expected": {}
    }

I have no reason to prefer one or the other, but if someone does, it would be good to mention the reason for that preference right now.

I also wouldn't consider having such tests a necessary condition to have this pull request be Approved (in the GitHub sense of the word), but I'll phrase it as: It wouldn't be surprising to see an additional pull request from me some time after this pull request is merged if this is indeed a desired teaching point.

[glennj]

I hear what you're saying about the teaching point.

Of these options, I prefer # 1. That indicates that we want to produce that result from the entire sequence of operations. Contrast to # 2 where the actual expected result of the test case is ambiguous.

I think we'd want to give some hint to track implementors that in this situation, the erroring operation has a non-fatal error or the error is to be trapped. Perhaps we could add

• a (new) scenario to the case or
• a "fatal": false property to the expected object of the middle operation.

[meatball133]

Sorry for taking a while, I will replay later today. Promise, aslong as I dont get a power outage but that usually doesnt happen.

[meatball133]

Okay I have read through the feedback. I think we can go with method one. But there are no current test cases which has any other operations in the middle so the fatal flag doesn't need to be applied anywhere. Unless you guys come up with some new tests. But I can convert the tests to suggestion 1. I can also write a comment about how we think about this design.

[glennj]

I'm satisfied with the current canonical data. @petertseng @SleeplessByte where do you stand now?

[petertseng]

Please amend the commit message to include the string "closes https://github.com/exercism/problem-specifications/issues/554" or any equivalent string. Thank you.

[meatball133]

I added so this pr will close that issue

[ErikSchierboom]

Thanks for working on this!

[ErikSchierboom]

Nice

[ErikSchierboom]

This is a new test case AFAICT, correct? This test case and a0a1835d-faae-4ad4-a6f3-1fcc2121380b are the two test cases of which I'm not sure about. Most existing implementations of this exercise don't have these check I think.

[meatball133]

Python has both of those tests, I can't speak although for how other tracks have done it.

[ErikSchierboom]

Ah, that's one implementation that I didn't do. Let's keep them then.

[meatball133]

@ErikSchierboom did you intend it like this?

[ErikSchierboom]

Just two nits, and then we should be good to go!

[ErikSchierboom]

Found just one tiny nit (sorry)

[ErikSchierboom]

LGTM!

[SleeplessByte]

Let's gooooo

[SleeplessByte]

@glennj please have a final look :)

[petertseng]

where is the open operation for this account?

[SleeplessByte]

cc @meatball133

[glennj]

@petertseng can you have a look here please

[petertseng]

if the consensus is that this is the best way to do things then I won't stand in your way. It strikes me as:

• inflexible (since now you can only have concurrent operations in between an initial and final section, instead of arbitrary interleavings)
• violating the schema (the same property, "bankAccount", now may have input of different types: it might either have operations, or not, violating the rule that the same property has input of the same type.)

What I expected to see was:

{
  "input": {
    "operations": [
      { 
        "operation": "open"
      },
      { 
        "operation": "concurrent",
        "number": 1000,
        "operations": [
          { 
            "operation": "deposit",
            "amount": 1
          },
          { 
            "operation": "withdraw",
            "amount": 1
          } 
        ] 
      },
      { 
        "operation": "balance"
      },
    ]
  },
  "expected": 0
}

[SleeplessByte]

I prefer this too. This also allows us to have multiple batches of concurrent operations instead of always serial start concurrent serial end.

[meatball133]

@glennj and @ErikSchierboom, I think we should have a solution we all agree upon so I want to hear your thoughts

[glennj]

I'm on board with this suggestion.

[ErikSchierboom]

Me too. That sounds like a better approach

[petertseng]

remember to properly edit the commit message when merging. We do not want to be discourteous to our future selves and future contributors by including a commit message with a bunch of lines that just say "fixed" and "updated"

[ErikSchierboom]

Thanks @meatball133!