Add charging_state to VehicleSummary

[brianmay]

Fixes: #3078

Was reverted in #3464, sorry, not sure what went wrong.

If there is a test case that needs updating, can somebody please point me to it?

[brianmay]

The problem appears to be because we receive a extra pubsub vehicle summary message that we weren't expecting.

But my code changes the code that receives the pubsub message, it shouldn't mean that there are additional pubsub messages being set.

Still puzzled.

[brianmay]

Does appear to be some intermittent test failures still.

Don't think that explains why this is failing though.

(currently this is succeeding because I commented out out of the lines).

[brianmay]

Cool, I think I broke lots of tests :-)

It is acting like there is some code somewhere that means the pubsub message will not get sent if it was the same value as the last message.

When I set the field to a constant, this means all tests pass (not counting intermittent faults).

But when I set the field to a random value, I get failures everywhere, because the summary keeps changing every time.

The only problem is I cannot find any code that dos this comparison.

I think for tests we are using our PubSubMock which just converts the PubSub message into a process message.

I don't see anything anywhere for dropping duplicate messages though.

[brianmay]

Sorry, I am an idiot, code is in test/support/mocks/pubsub.ex:

  def handle_call({:broadcast, _, _, _} = event, _from, %State{last_event: event} = state) do
    {:reply, :ok, state}
  end

  def handle_call({:broadcast, _, _, _} = event, _from, %State{pid: pid} = state) do
    send(pid, {:pubsub, event})
    {:reply, :ok, %State{state | last_event: event}}
  end

When we receive and event we save it to last_event. Then if the next event matches, we ignore it. This only happens during tests.

Question is how do we fix this? If I just added extra assert_receive lines in the tests in test/teslamate/vehicles/vehicle/charging_test.exs - sure that would fix the problem. But it seems to be a very fragile solution.

[brianmay]

That error I just got looks kind of bad:

** (UndefinedFunctionError) function System.convert_time_unit/3 is undefined or private
    (elixir 1.15.7) System.convert_time_unit(-576460734009829459, :native, :millisecond)

Huh?

Source: https://github.com/teslamate-org/teslamate/actions/runs/6936413423/job/18868513844

[brianmay]

OK, tests pass, but not convinced this is a good solution...

[JakobLichterfeld]

Sorry, I am an idiot, code is in test/support/mocks/pubsub.ex:

  def handle_call({:broadcast, _, _, _} = event, _from, %State{last_event: event} = state) do
    {:reply, :ok, state}
  end

  def handle_call({:broadcast, _, _, _} = event, _from, %State{pid: pid} = state) do
    send(pid, {:pubsub, event})
    {:reply, :ok, %State{state | last_event: event}}
  end

When we receive and event we save it to last_event. Then if the next event matches, we ignore it. This only happens during tests.

Question is how do we fix this? If I just added extra assert_receive lines in the tests in test/teslamate/vehicles/vehicle/charging_test.exs - sure that would fix the problem. But it seems to be a very fragile solution.

Great find, thanks for the in-depth research!

[brianmay]

Would this be considered acceptable as is?

Not really sure how to make the tests more robust without making them more complicated.

I think they key issue is that these tests only want changes where the :state and/or :since values are changed. But instead we get all changes.

I guess we could change the pubsub mock, but that seems yuck. We might want to write a test that tests other values in the future.

We could also change the assert_receive with a function that loops over the results until it gets a change that it is looking for. That seems overly complicated; tests need to be simple and easy to understand.

[JakobLichterfeld]

I completely agree.

[JakobLichterfeld]

ty.