fix: make algorand client persistent for test fixture #356
Conversation
|
This is seemingly breaking tests inconsistently but not entirely sure why. I can investigate further but if anyone has some insight that would be appreciated. @robdmoore do you know why this was changed in the first place? |
|
@joe-p Yeah that's why it was changed. Because AlgorandClient is stateful, it's possible for a test to change the AlgorandClient internal state, whilst another test is also being executing. I'm assuming it's whilst an async function is being awaited or something like that. |
|
Ok a couple of things
beforeEach(async () => {
await fixture.beforeEach()
// Propagate signers from any `beforeAll` calls
fixture.algorand.account.setSigners(validatorMasterAlgorandClient.account)
// Register the test account for this test with the validatorMasterClient
validatorMasterAlgorandClient.setSignerFromAccount(fixture.context.testAccount)
// Register any generated test accounts for this test with the validatorMasterClient
const generator = fixture.context.generateAccount
fixture.context.generateAccount = async (params) => {
const account = await generator({ initialFunds: params.initialFunds, suppressLog: true })
validatorMasterAlgorandClient.setSignerFromAccount(account)
return account
}
})As such we should try to fix this behavior. In 5918017 I have fixed the problems caused by the race conditions, but the tests are failing locally for me because |
|
Once #358 is merged I can rebase this branch which should make it ready to merge |
|
I'm honestly not sure that we should be caching the Additionally with this change, there isn't a way to easily reset the Would it work in the reti case to run |
|
This is an undocumented breaking change that is blocking developers from updating. That alone I think is a good enough reason to revert it back. Developers I've talked (and myself) to expect the fixture to be persistent (like pytest fixtures) which is why this was made persistent in v6 in the first place. If we want to expose a way to generate a new client we can add a function to the fixture context. |
|
Playing devils advocate, this change to revert to the v6 behaviour would introduce a breaking change to both v7 and v8, so could impact devs who have migrated from pre v6 versions to v7/8. I just want to make it clear that in the current state (unless I'm missing something), developers can control the lifetime of the @lempira Any thoughts on this one? |
|
I think v7/v8 is new enough that it is an acceptable change to make, especially considering it was undocumented. If we need to release as v9 then so be it. It's better to restore functionality than try to stay on v8.
The problem is that tests might be using other parts of the fixture, such as |
|
I started work on upgrading my NFD contracts and tests to algokit v8. I've wasted at least two days to no avail - it's completely broken and I've stopped entirely. It just takes one look at that monstrosity of a before call that was added to Reti that Joe quoted above as part of v6->v7 integration to say that none of that has ANY business being in a test for end-user developers. If I create test accounts as a part of tests, often I'll have a series of tests that build upon state (because the state itself might be complex or time consuming to create - so its layered). Any accounts created, should be able to be signed anywhere after that point. In tests there's no reason to reset keys nor is there a risk. We're not testing vulnerabilities of in-memory key storage here guys. |
|
A few comments, in no particular order:
|
|
I haven't delved deep into the utils-ts code and tests, particularly for the v7/8 release, so take my comments and suggestions in that context. From what I understand, the reason for removing the client's persistence from the fixture was to better control the client state when setting up and tearing down each test. If that's the case, then I agree with the general philosophy that it's better to have better control of a fixture test state over what the test may look like. The reason is that I have been burnt many times by flaky tests that work locally and don't in CI because they are usually run in multiple threads where the order of the tests usually matters because tests are trying to access or mutate the same state. I agree that the snippet that Joe added looks more gnarly, but we could make composable fixtures that abstract the most common setups so they don't look so complex when used in each test. It would also allow the reuse of those different setups as necessary. I am surprised that client persistence (non-determinism) hasn't been an issue before. It sounds like the tests are indeed idempotent despite having that shared state. Perhaps this is because each account that is created in the tests is different and thus is isolated from other tests. This makes me less wary to revert the change as proposed in the PR. I agree with Joe since this testing fixture is used in many places and seems to have effects on external repos like RETI; this should have been documented as part of the major breaking changes. Given the importance of RETI to staking rewards, I don't think we shouldn't add additional friction to the deployment, unless it's something critical, which I don't deem this to be. Suggested path forward:
Other questions re: this PR
|
|
I was playing around this a little more. I merged #358 to this branch, and the tests passed, but I am going to amend my suggested path forward. One thing I noticed with this approach is that it ties you into having the AlgorandClient run in the global scope of tests. If you did want to have a test run standalone so that it sets up and tears down in a beforeEach, for example, you wouldn't be able to use this fixture because it would have the state and particularly the previous transaction logs from the previously run tests. It would be better to have the flexibility to determine the scope of the fixture that is appropriate for your test. This is particularly needed now since v7/v8 introduced the stateful AlgorandClient. Perhaps the 'beforeEach' fn in the current implement is a misnomer and could be replaced with something that would let the dev know that you can use that in whatever scope you want. I think that's what Neil was suggesting above Since this thread is long, I am going to set up a meeting on Thursday morning to discuss this. It might be easier to talk through the details. |
|
Finally had a chance to look into this. Apart from renames, from the v6 version with AlgorandClient the following were the key changes that were made in relation to
An inadvertent change from this was what is raised here, which is technically that was a different behaviour to what was in v6 so it should have been mentioned in the release notes. This was unintentional and was simply missed given the sheer number of changes being made along with the fact that It's fair that there are situations where it may be useful to share an To David's point, both behaviours are potentially useful in different circumstances so what makes the most sense is what he suggested - that this is a configurable behaviour. As such, I've put up a pull request #359 which has an implementation that provides this functionality. It does this by allowing a new configuration to describe('MY MODULE', () => {
const fixture = algorandFixture({ algorandScope: 'fixture' })
beforeEach(fixture.beforeEach, 10_000)
test('test1', async () => {
const { algorand, transactionLogger, testAccount } = fixture.context
// ...
})
test('test2', async () => {
const { algorand, transactionLogger, testAccount } = fixture.context
// algorand and transactionLogger are the same as test1, testAccount is different
})
})By default it makes sense to encourage test isolation so if you don't specify it then we assume I added a test to illustrate it: https://github.com/algorandfoundation/algokit-utils-ts/blob/algorand-fixture-scoping/src/testing/fixtures/algorand-fixture.spec.ts. The problem with the |
|
@pbennett re your comments:
Can you be more specific please so we can address specific bits of feedback. This particular issue raised by Joe is fair feedback, but as you can see above this was an unintended side-effect not a deliberate change and there's a straightforward way to resolve it once we merge #359 where you can find all
I'm sorry you feel that way. We did the best we could to get betas out and ask for feedback, including from yourself and devrel and made many changes as a result of that feedback and also used it on all of our codebases to try it out. I personally went out of my way to help you get this stuff working in reti and take on board your feedback and adjust the libraries based on it. The comments we received from you were that the end result of the changes were a lot clearer than the previous versions of utils. I acknowledge that you've been on the bleeding edge of this stuff, and were using the beta versions of algorand client in v6 as an early adopter, which we appreciate, but naturally is a more painful experience than someone following the bouncing ball now it's all released as non-beta. We've put a lot of effort into making a migration experience from stateless functions to algorand client smooth and did our best to minimise breaking changes for v6 beta usage of algorand client in v6, but unfortunately had to make a series of breaking changes to get the right end result (again, largely based on feedback). Ultimately, this series of changes are very complex, it's a big set of changes and hard to get perfect, but we remain open to and reactive to tangible feedback and truly believe the end result is a much better library for interacting with Algorand.
I'm sorry, but I have to disagree with you there. Raw alsosdk is very low level and clunky to use. But in saying that, if you have specific examples happy to hear them so we can keep iterating based on feedback as we have already been doing.
Agreed, but that isn't the code that makes sense to write and #359 will nullify the need for that kind of code and allow you to do the kind of shared setup state that you want to do easily. |
|
My frustration stemmed from wasting 2 days trying to figure out (unsuccessfully) why code that worked fine stopped working, combined with having to convert everything over to the complete algokit v6->[v7|v8] overhaul. Signing not working for no obvious reason, logging failing with bigint errors (in the framework) that weren't there before (and so not seeing any issues at all). It seems like I break something every time I try any new version so it feels like what I'm using hasn't actually been tested. Documentation is basically non-existent and between my inexperience w/ JS/TS, its test frameworks and what is/isn't algokit's bit of 'opinion' on how it should/shouldn't be done it feels like I'm mostly throwing darts at things hoping it works that I shouldn't have to. Converting Reti from v6 to v7 then v8 was beyond painful. You helped immensely w/ the v6 to v7 - but didn't even touch the UI part of it and I think you probably realized how much of a change it was for people. So going from v6 to v8 - with even a little bit of a code - you basically have to touch everything. Even in your example, I see problems: describe('MY MODULE', () => {
const fixture = algorandFixture({ algorandScope: 'fixture' })
beforeEach(fixture.beforeEach, 10_000)
test('test1', async () => {
const { algorand, transactionLogger, testAccount } = fixture.context
// ...
})
test('test2', async () => {
const { algorand, transactionLogger, testAccount } = fixture.context
// algorand and transactionLogger are the same as test1, testAccount is different
})
})the beforeEach that calls.. beforeEach with 10,000 argument (ok? 10,000 ... what?) I would expect something more like this: describe('MY MODULE', () => {
const { algorand, transactionLogger, testAccount } = algorandFixture().context
test('test1', async () => {
// ...
})
test('test2', async () => {
})
})If I want to generate new accounts, I will generate new accounts. I 100% do not expect every test to literally create a new accounts nor start in a 'clean state'. I control that with my scoping already. We're testing contracts - with state. It's like you're expecting every test to literally re-bootstrap everything which doesn't match at all how you need to test real-world contracts. |
These are a mischaracterization of the bug and its relation to the AlgorandClient. The current implementation of the transaction logger and its I think this is worth pointing out because it is a key factor in the crux of the issue here. This change was made based on a false premise. I believe that for any change, especially breaking ones, we should be able to map the change to a problem it is solving. Even more importantly, we should be able to map that problem back to users otherwise we will find ourselves over-engineering. Obviously missing the bug in #358 was an oversight but once the change is made and the affects downstream become evident (i.e. the above Reti code) there should be some further though on whether it really is the best path forward. I see points being raised about determinism, but it's not really clear to me what the concern is. What is the state persisted by AlgorandClient that is believed to be potentially harmful to testing? This is a breaking change, so I think we should be very clear in why it's being made. I also think a key issue here is naming. |
As I said, I totally understand your frustration and appreciate you've felt it the most since you've stayed at the bleeding edge the whole way through. The bigint thing was because algosdk 3 introduced bigints into core objects that previously weren't there so there was never a need to use a custom replacer in
If you are going from v6 pre-AlgorandClient (i.e. stateless functions) then no, you don't need to touch anything* and you can do a gradual migration bit by bit. Usage of AlgorandClient in v6 for sure would have required some changes, but again, we did try and minimise that where we could and create a migration guide to describe what changes needed to be made. *Outside of the breaking changes in algosdk@3 like
The
A core part of good practice testing is isolation between tests, so yes a clean state should be the default, and all of the tests we have written across various projects generally make use of that. Of course in some instances you may want to have shared state / setup and there's a number of ways to do that using the testing frameworks. Two examples below that will work in the way you want with the new change I put up yesterday (these will actually also both work with the current implementation on npm too without the new describe('MY MODULE', () => {
const fixture = algorandFixture({ algorandScope: 'fixture' })
let algorand: AlgorandClient
let testAccount: Address
beforeAll(async () => {
await fixture.beforeEach() // Run beforeEach once only (in beforeAll) so the context doesn't change across tests
;({ algorand, testAccount } = fixture.context)
}, 10_000)
test('test1', async () => {
console.log(testAccount)
})
})
describe('MY MODULE', () => {
const fixture = algorandFixture({ algorandScope: 'fixture' })
beforeAll(fixture.beforeEach, 10_000) // Run beforeEach once only (in beforeAll) so the context doesn't change across tests
test('test1', async () => {
const { algorand, testAccount } = fixture.context
console.log(testAccount)
})
})
describe('MY MODULE', () => {
const { algorand, transactionLogger, testAccount } = algorandFixture().context
test('test1', async () => {
// ...
})
test('test2', async () => {
})
})Unfortunately, that kind of syntax isn't possible with vite/jest because the fixture has to be async and I don't believe you can have an async definition function. |
It's probably worth describing in detail how this change came to be, as it's actually not a mischaracterization. The reason this change was originally made was for a different reason to what you are mentioning. We started noticing weird test failures inside utils-ts whilst working on the v7 branch and writing more tests leveraging the AlgorandClient. The issue experienced was intermittent test failures when testing functions like In investigating this issue, we realised it was related to the scoping of the const beforeEach = async () => {
// ...
const transactionLogger = new TransactionLogger()
const transactionLoggerAlgod = transactionLogger.capture(algod)
algorand = algorand ?? AlgorandClient.fromClients({ algod: transactionLoggerAlgod, indexer, kmd })
// ...
context = {
// ...
transactionLogger: transactionLogger,
waitForIndexer: () => transactionLogger.waitForIndexer(indexer),
waitForIndexerTransaction: (transactionId: string) => runWhenIndexerCaughtUp(() =>
indexer.lookupTransactionByID(transactionId).do()),
}
}Because Our frame of context at the time was that we were fixing a bug and correcting the lifetime scoping to match our documented usage. In hindsight I can definitely see your point that it is behaviourally a breaking change that should have been captured in the v6 -> v7/v8 migration guide. Which we can totally still do. The problem you noticed in this PR, which you added the commented todo block of code for is due to the above.
In reading this it made me realised that we hadn't done a good job telling the story of how the change came to be. Hopefully my description clarifies. The change wasn't made based on a false premise, and I believe that v6 actually still has the flaw.
Test fixture is a colloquial term to define a test context. The purpose is to ensure a fixed, deterministic environment for the test rather than always having a singleton lifetime/scope. I definitely think it's useful to be able to control the lifetime/scope. Pytest fixtures for example have a default 'function' (per test) scope, but can be controlled depending on your needs. |
This isn't true. Literally every contract call had to change in the generated clients for example. .compose() became newGroup(). All the .send additions. send instead of execute.. args not being the first parameter but now being part of embedded object as 'args:', fee definitions changing, etc. etc. It was basically a touch everything process.
Sure, but the developer should decide where that isolation is, not you. Smart contracts require state and in some cases that state might literally require hundreds of transactions to create properly - specific balances, specific accounts, box state, etc. etc. That should NOT be done PER TEST. For some? Sure. For all? Absolutely not - and that's not the frameworks job to force on me. The framework is literally overriding scope expectations and expecting me to 'override' it to make it work like expected.
You listed two examples that seem to show having to do extra (and non-intuitive) work to make it.. stay out of my way. 😞 A fixture that I have to tell to scope to .. a fixture ? |
You could have avoided this by using the previous version of the typed client generator and keeping the existing typed clients while migrating gradually. Because v7/v8 was backwards compatible to v8 for stateless functions (which the previous version of the typed clients used) they would still work. @neilcampbell did we mention this in the migration guide? If not we should add it.
That's what I proposed in the PR I raised yesterday - that the developer can control the scope?
The first example, sure (but I only included it because it was the closest to the syntax you suggested where there are test global variables so you don't have to destructure in every test - and the pattern I included is typical jest/vitest code for doing that kind of thing though, not something we are forcing on people), the second example I gave though is exactly the same as the typical example except
The reason I used |
|
I feel like we are not too far from a decision to make the experience more flexible for devs and solve the issues mentioned in this thread. It'd be good to try make this change as backwards compatible as possible, so we can update both v7/v8. The options as I see them are: Option 1 - Do nothingAlways an option worth calling out, however in this case I'm not sure it's desirable. Developers who want to have fixture per test suite scoping can use the below. const fixture = algorandFixture()
beforeAll(fixture.beforeEach)This would be documented clearly and we'd explicitly call out the fact that Per test semantics would remain as already documented in the repo. Pros
Cons
Option 2 - Cache AlgorandClient inside the fixtureThis option would be largely based on #356 and #358 which caches the Any calls to Developers who want to have fixture per test suite scoping can use the below. const fixture = algorandFixture()
beforeEach(fixture.beforeEach)Developers who want to have fixture per test scoping can use the below. let fixture = algorandFixture()
beforeEach(async () => {
fixture = algorandFixture()
await fixture.beforeEach()
})Pros
Cons
Option 3 - Rename
|
No actually I couldn't - since I needed latest arc56 support etc - which means you have to kind of lock-step go with latest of everything (which I wanted to do). srcmaps have been completely broken and I was hoping the latest and greatest would maybe fix it along with getting current w/ sdk v3, etc. |
|
Thanks for creating these options @neilcampbell. They help gain a better understanding of the landscape for a path forward. I am in favor of option 3. I agree with Joe that the As in option 1, I also am in favor of adding additional docs regarding testing that would explain how to use this fixture and perhaps a recommendation on test isolation. |
|
@pbennett is there an open issue for the source map thing you mentioned? |
By the way, the main issue was that because the algorand client was globally scoped and all transactions were being logged from all previous test runs (sure, including potentially failed ones) and the current implementation of By changing We could of course replace to call to |
|
FYI I've added a proposed pull request that implements option 3 along with something that propagates registered signing accounts with each successive newScope call to avoid potential lack of signer troubles when using multiple scopes together (e.g. when deciding to create a scope in a global context and different scopes in per-test contexts). |
It's something I've talked w/ Joe about. The existing mapping code is set up to handle simple single contract calls, not contracts that call other contracts (where the failure is in that nested call) as lookup is going to be in the context of a simple 'client X calls contract X'. Kind of independent of this PR though. The explict newContext option 3 is definitely a better choice. Reading this PR I learned that algokit was repeatedly generating 'new' test accounts for me nonstop as well (which I never asked or expected it to do). Explains a lot of issues I noticed. I'd simply expect a new test account whenever I explicitly generated one. I assumed getting testAccount from the fixture was getting me the same test account - not generating a new one. With all the comments about the intended design ideals of per-test clean-slate contract testing, could someone point me to what they consider a good production example of this w/ algokit and algorand smart contracts? I'd really like to see these contracts and what they do to have clean-slate testing (for 'live' integration tests!) be a preferred approach. |
|
@pbennett it's a bit older so isn't using typed clients, but this example shows isolated tests: https://github.com/algorandfoundation/nft_voting_tool/blob/develop/src/algorand/smart_contracts/tests/voting.spec.ts It's worth pointing out, there's nothing wrong with setting up common state across a series of tests - if it takes a lot of time to do and the individual tests are able to execute with relative isolation from each other i.e. you know each tests is in a clean state of some sort then it's fine. The problem occurs when tests aren't isolated from each other and tests fail when executed in a different order etc. The good thing about the option 3 suggestion is it makes it clear that the scoping of the fixture context is up to the developer (which it was already). For instance, here's a test in utils-ts itself that used |
Yes sorry I had forgotten that there was two issues.
I just want to make it clear to readers that problems with the AlgorandClient being global is NOT inherit to the AlgorandClient being global. #358 solves this but does also change the scope but that is not crucial to the solution. An alternative solution would be popping transactions that are already waited for successfully and popping transactions that are failed but #358 seemed like the simpler solution. Regardless of the solution here, I think we need to fix these issues. I can create an issue for this effort (created #363).
Problems could arise if you are doing something with a
I'm confused on why this would be the case... "wait for all transactions in algod to show up" and "wait for the last round to show up" are the same questions as far as indexer is concerned. I don't want to sidetrack the main discussion here, so I've created #363 to continue the discussion. I agree 3 is the best solution. I also think for everything exposed through the context there should be a helper function for generating a new instance. Similar to how there's |
Proposed Changes