fix: pricing integration tests -> trying more runs for cache and retries

[michaelneale]

these tests seem to fail a bit - due to availability of an api, and also testing with microseconds for cache timing, so runs many iterations of latter to check caching is faster (is a bit of an odd test in the first place to test in CI)

[jsibbison-square]

Personally I think some these of tests don't add a lot of value and we should delete all except for 'model_in_open_router', 'model_not_in_open_router' and maybe the concurrent one.

• test_pricing_cache_performance Checking performance in CI doesn't make sense as we are not comparing against historic time and we will write off failures as transient issues
• test_model_not_in_openrouter Valid behavior test
• test_pricing_refresh I think this test is just testing that the refresh_pricing method exists, no assertions of granuality
• test_concurrent_access This is ensuring we are using a mutex in the code, no easy way to test it that I can think of.

[michaelneale]

@jsibbison-square agree - yeah I don't think they add a lot, could drop some of them?

[lifeizhou-ap]

Hi @michaelneale and @jsibbison-square ,

I had the look into the build failure reason here. I feel below is the the reason why the test is not stable.

In our tests, we use #[tokio::test]. By default, each test run on a multi-threaded runtime, which are running parallel with the other test cases.

let test_cache_dir = format!("/tmp/goose_test_cache_perf_{}", std::process::id());
std::env::set_var("GOOSE_CACHE_DIR", &test_cache_dir);

With the above code, each tests created same directory as a cache dir because they shared the same process id and the same environment variable. In each test cases, it set the cache_dir first and remove the dir. However, since they are using the same dir name, it causes each test case use a shared file which caused the failure when running in parallel.

i guess the options to fix instabilities and make sure test isolation are:

• passing cache_dir in the PricingCache struct constructor to make it is easier to test
• change the test to run in sequential (I don't think this is a good solution. each tests case should be isolated from each other)
• using temp_env crate to override the environment variable in the test.

I also went through the test cases (as integration tests)

• test_pricing_cache_performance:
  My understanding of the intention for this test case: when getting the price first, it fetches the prices and build the cache, and when getting the price again, it uses the cache. I feel this is a valid test scenario. can verify the data with the cache file instead of the time elapse

• test_model_not_in_openrouter is valid scenario

• test_pricing_refresh is also a valid scenario. It can check the timestamp in the cache file

• test_concurrent_access, it could be a valid test case, but I am not sure how to verify (maybe make sure it can get the model price at the end of test again?)

I feel when we use external services in the test cases, it also brings instability. maybe some of them can be converted to unit tests, and we can have a single integration test.

[michaelneale]

even some retries on external may be ok, and testing cache as one shot is not right, need to let it run some iterations (comparison at microsecond level does seem a bit meaningless for that test though)

[michaelneale]

thanks @lifeizhou-ap switched to using proper tempfile - that should help with some, still has the retries in there and gives the cache a good number of runs to ensure it gets faster to stick to intent of test (others are in there).