Add unit tests for bytecode arguments reading

[raskad]

We should have unit test for writing / reading of bytecode arguments. Especially during argument reads we use unsafe so unit tests should ensure that every read can only panic.

Argument writing / reading is confined to args.rs.
Tests should cover every argument type and if present format.

[t-h-p]

Could I be assigned this?

[HalidOdat]

Could I be assigned this?

Sure, go ahead 😄

[t-h-p]

Would it be preferable to write tests using encode/decode or the read and write functions directly? As of now the tests follow the form:

  #[test]
  fn writing_arg_u8() {
    let mut v: Vec<u8> = Vec::new();
    let val = VaryingOperand{value: u8::MAX as u32};
    val.encode(&mut v);
    assert_eq!(vec![0,0xFF], v);
  }
  #[test]
  fn reading_arg_u8() {
    let bs: Box<[u8]> = Box::new([0,0xFF]);
    let r: &[u8] = &bs;
    assert_eq!(
      VaryingOperand{value: u8::MAX as u32}.value,
      VaryingOperand::decode(r,0).0.value
    );
  }
  #[test]
  #[should_panic]
  fn short_fail_reading_arg_u8() {
    let bs: Box<[u8]> = Box::new([0]);
    let r: &[u8] = &bs;
    VaryingOperand::decode(r, 0);
  }

[raskad]

I think they should use the encode / decode APIs, because those are used from "outside". In the tests we don't really care what the bytecode looks like, as long as the encoding / decoding is safe / works.

[t-h-p]

Not sure if this is the correct issue to ask in, but is it intended for decode::<(VaryingOperand, i32)> to not account for format?

[HalidOdat]

Not sure if this is the correct issue to ask in, but is it intended for decode::<(VaryingOperand, i32)> to not account for format?

Oh, I see, that's a mistake, nice catch, all the ones that have VaryingOperand should account for format.

EDIT: Context: https://github.com/boa-dev/boa/blob/main/core/engine/src/vm/opcode/args.rs#L287-L297

[t-h-p]

I'll fix that in my changes as well.

[t-h-p]

Sorry it took a while to get back to this, I made the PR but there are still some VaryingOperand related encode/decode functions that don't account for formatting. I wasn't sure how to address them as they also included two bytes for length of a ThinVec. The ones I am thinking of in particular are for (u64, VaryingOperand, ThinVec<u32>) and (VaryingOperand, VaryingOperand, ThinVec<VaryingOperand>) but I am pretty sure there are a couple others as well. Would we want to include the format before or after the length bytes?

[ParthMozarkar]

could you assign this issue to me?